Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make enableOwnerStacks dynamic #31661

Merged
merged 13 commits into from
Dec 11, 2024
Merged

Make enableOwnerStacks dynamic #31661

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
afc3e7e
Make enableOwnerStacks dynamic
rickhanlonii Oct 18, 2024
c198032
Export captureOwnerStack from fb entrypoint
rickhanlonii Oct 18, 2024
ba4f668
Flag export for captureOwnerStack
rickhanlonii Oct 18, 2024
7879dbc
fix stack gating for ReactStrictMode-test
noahlemen Dec 2, 2024
25b3193
fix flow, fix creteReactClassIntegration test
noahlemen Dec 3, 2024
db722ee
fix failing ReactFlight test
noahlemen Dec 3, 2024
ff18dfc
only export captureOwnerStack on server when enabled
noahlemen Dec 3, 2024
2408f1b
revert change to createReactClassIntegration-test
noahlemen Dec 3, 2024
75ce892
Revert "revert change to createReactClassIntegration-test"
noahlemen Dec 3, 2024
da99921
remove changes to ReactServer.fb.js
noahlemen Dec 4, 2024
223b2a2
actually ReactServer.fb needs the export
noahlemen Dec 5, 2024
0170f5f
gate export on ReactServer.fb.js
noahlemen Dec 9, 2024
d82a7b4
gate export on __DEV__ and flag
noahlemen Dec 11, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions packages/react/index.fb.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@
* @flow
*/

import {enableOwnerStacks} from 'shared/ReactFeatureFlags';
import {captureOwnerStack as captureOwnerStackImpl} from './src/ReactClient';
export {
__CLIENT_INTERNALS_DO_NOT_USE_OR_WARN_USERS_THEY_CANNOT_UPGRADE,
__COMPILER_RUNTIME,
Expand Down Expand Up @@ -68,3 +70,11 @@ export {useMemoCache as unstable_useMemoCache} from './src/ReactHooks';
// export to match the name of the OSS function typically exported from
// react/compiler-runtime
export {useMemoCache as c} from './src/ReactHooks';

// Only export captureOwnerStack in development.
let captureOwnerStack: ?() => null | string;
if (__DEV__ && enableOwnerStacks) {
captureOwnerStack = captureOwnerStackImpl;
}

export {captureOwnerStack};
9 changes: 9 additions & 0 deletions packages/react/src/ReactServer.fb.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,8 @@
export {default as __SERVER_INTERNALS_DO_NOT_USE_OR_WARN_USERS_THEY_CANNOT_UPGRADE} from './ReactSharedInternalsServer';

import {forEach, map, count, toArray, only} from './ReactChildren';
import {enableOwnerStacks} from 'shared/ReactFeatureFlags';
import {captureOwnerStack as captureOwnerStackImpl} from './ReactOwnerStack';
import {
REACT_FRAGMENT_TYPE,
REACT_PROFILER_TYPE,
Expand Down Expand Up @@ -37,6 +39,12 @@ const Children = {
only,
};

// Only export captureOwnerStack if the flag is on, to support feature detection.
let captureOwnerStack: ?() => null | string;
if (__DEV__ && enableOwnerStacks) {
captureOwnerStack = captureOwnerStackImpl;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is sus, why does this need exported here but not from the other ReactServer.js exports?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in your PR it was added without the flag check, which seemed to break feature detection in the tests. so i added the gating, which corrected it.

you're right though, i hadn't noticed that ReactServer.js doesn't export it at all. looks like just not changing this file at all would be a valid approach too.

Copy link
Member Author

@noahlemen noahlemen Dec 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

after investigating this a bit more, i think exporting this dynamically based on the feature flag is correct.

in the experimental builds we have a dedicated file for the development builds (ReactServer.experimental.development vs ReactServer.experimental, index.experimental.development vs index.experimental) and only export captureOwnerStack in the development version.

the *.fb files do not have separate development versions, so if we want parity with experimental, we should gate defining the export on __DEV__.

however, if we gate based on just that, we still get some failures. there's another subtle inconsistency - in experimental builds the flag is always enabled. here, we use __VARIANT__, which creates a situation where, if we gate only based on __DEV__ we could export a defined captureOwnerStack but have enableOwnerStacks disabled (eg: when tests are run with --variant=false).

because some of the existing test infrastructure code (1, 2) for handling this operates based on feature detection of captureOwnerStack being defined, this is very consequential for how the tests are ultimately run.


export {
Children,
REACT_FRAGMENT_TYPE as Fragment,
Expand All @@ -57,4 +65,5 @@ export {
useDebugValue,
useMemo,
version,
captureOwnerStack, // DEV-only
};
95 changes: 61 additions & 34 deletions packages/react/src/__tests__/ReactStrictMode-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1028,40 +1028,67 @@ describe('context legacy', () => {
root.render(<Root />);
});

assertConsoleErrorDev([
'LegacyContextProvider uses the legacy childContextTypes API ' +
'which will soon be removed. Use React.createContext() instead. ' +
'(https://react.dev/link/legacy-context)' +
'\n in LegacyContextProvider (at **)' +
'\n in div (at **)' +
'\n in Root (at **)',
'LegacyContextConsumer uses the legacy contextTypes API which ' +
'will soon be removed. Use React.createContext() with static ' +
'contextType instead. (https://react.dev/link/legacy-context)' +
'\n in LegacyContextConsumer (at **)' +
'\n in div (at **)' +
'\n in LegacyContextProvider (at **)' +
'\n in div (at **)' +
'\n in Root (at **)',
'FunctionalLegacyContextConsumer uses the legacy contextTypes ' +
'API which will be removed soon. Use React.createContext() ' +
'with React.useContext() instead. (https://react.dev/link/legacy-context)' +
'\n in FunctionalLegacyContextConsumer (at **)' +
'\n in div (at **)' +
'\n in LegacyContextProvider (at **)' +
'\n in div (at **)' +
'\n in Root (at **)',
'Legacy context API has been detected within a strict-mode tree.' +
'\n\nThe old API will be supported in all 16.x releases, but applications ' +
'using it should migrate to the new version.' +
'\n\nPlease update the following components: ' +
'FunctionalLegacyContextConsumer, LegacyContextConsumer, LegacyContextProvider' +
'\n\nLearn more about this warning here: ' +
'https://react.dev/link/legacy-context' +
'\n in LegacyContextProvider (at **)' +
'\n in div (at **)' +
'\n in Root (at **)',
]);
if (gate(flags => flags.enableOwnerStacks)) {
assertConsoleErrorDev([
'LegacyContextProvider uses the legacy childContextTypes API ' +
'which will soon be removed. Use React.createContext() instead. ' +
'(https://react.dev/link/legacy-context)' +
'\n in Root (at **)',
'LegacyContextConsumer uses the legacy contextTypes API which ' +
'will soon be removed. Use React.createContext() with static ' +
'contextType instead. (https://react.dev/link/legacy-context)' +
'\n in LegacyContextProvider (at **)' +
'\n in Root (at **)',
'FunctionalLegacyContextConsumer uses the legacy contextTypes ' +
'API which will be removed soon. Use React.createContext() ' +
'with React.useContext() instead. (https://react.dev/link/legacy-context)' +
'\n in LegacyContextProvider (at **)' +
'\n in Root (at **)',
'Legacy context API has been detected within a strict-mode tree.' +
'\n\nThe old API will be supported in all 16.x releases, but applications ' +
'using it should migrate to the new version.' +
'\n\nPlease update the following components: ' +
'FunctionalLegacyContextConsumer, LegacyContextConsumer, LegacyContextProvider' +
'\n\nLearn more about this warning here: ' +
'https://react.dev/link/legacy-context' +
'\n in Root (at **)',
]);
} else {
assertConsoleErrorDev([
'LegacyContextProvider uses the legacy childContextTypes API ' +
'which will soon be removed. Use React.createContext() instead. ' +
'(https://react.dev/link/legacy-context)' +
'\n in LegacyContextProvider (at **)' +
'\n in div (at **)' +
'\n in Root (at **)',
'LegacyContextConsumer uses the legacy contextTypes API which ' +
'will soon be removed. Use React.createContext() with static ' +
'contextType instead. (https://react.dev/link/legacy-context)' +
'\n in LegacyContextConsumer (at **)' +
'\n in div (at **)' +
'\n in LegacyContextProvider (at **)' +
'\n in div (at **)' +
'\n in Root (at **)',
'FunctionalLegacyContextConsumer uses the legacy contextTypes ' +
'API which will be removed soon. Use React.createContext() ' +
'with React.useContext() instead. (https://react.dev/link/legacy-context)' +
'\n in FunctionalLegacyContextConsumer (at **)' +
'\n in div (at **)' +
'\n in LegacyContextProvider (at **)' +
'\n in div (at **)' +
'\n in Root (at **)',
'Legacy context API has been detected within a strict-mode tree.' +
'\n\nThe old API will be supported in all 16.x releases, but applications ' +
'using it should migrate to the new version.' +
'\n\nPlease update the following components: ' +
'FunctionalLegacyContextConsumer, LegacyContextConsumer, LegacyContextProvider' +
'\n\nLearn more about this warning here: ' +
'https://react.dev/link/legacy-context' +
'\n in LegacyContextProvider (at **)' +
'\n in div (at **)' +
'\n in Root (at **)',
]);
}

// Dedupe
await act(() => {
Expand Down
9 changes: 8 additions & 1 deletion packages/react/src/__tests__/createReactClassIntegration-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -338,7 +338,14 @@ describe('create-react-class-integration', () => {
root.render(<Outer />);
});
assertConsoleErrorDev([
'Component uses the legacy childContextTypes API which will soon be removed. Use React.createContext() instead.',
gate(flags =>
flags.enableOwnerStacks
? [
'Component uses the legacy childContextTypes API which will soon be removed. Use React.createContext() instead.',
{withoutStack: true},
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this doesn't seem like it should be needed, still working on figuring out what's going on here

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

think this may actually be correct. the error below for contextTypes has an owner stack because it is on Foo, which is rendered by Outer. childContextTypes is on Outer, however, which has no owner – it is rendered directly by root.render. If Outer is wrapped with an arbitrary wrapper component, we get a stack here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why wasn't this failing before though?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like a bug in the checker to me. Why do we need to gate only first message?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hoxyq see my second comment - it seems there's no "owner" to point to (it would be the root.render call on L338), so there's no stack

@rickhanlonii it wasn't failing before because owner stacks weren't enabled until this PR, right?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see my second comment - it seems there's no "owner" to point to (it would be the root.render call on L338), so there's no stack

Makes sense, but when enableOwnerStacks is disabled, classic component stacks should be appended, which will be empty in this case as well, but was working well without specifying withoutStack property.

Copy link
Member Author

@noahlemen noahlemen Dec 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that's true - it appears that with enableOwnerStacks disabled the component stack here ends up pointing at that anonymous function. not sure if we should expect owner stacks to do the same?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that's true - it appears that with enableOwnerStacks disabled the component stack here ends up pointing at that anonymous function. not sure if we should expect owner stacks to do the same?

oh, now I get it, so non-empty component stack gets appended, if feature flag is disabled, but once its enabled, then the owner stack is empty and its basically a false-positive

thanks for the explanation

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why wasn't this failing before though?

to fully close the loop on this, @rickhanlonii, it seems its just because with enableOwnerStacks prior only enabled in experimental, disableLegacyContext was always enabled

since this test has // @gate !disableLegacyContext, it plainly was never running before when enableOwnerStacks was gated, as there wasn't a permutation of the feature flags where disableLegacyContext was false but enableOwnerStacks was true. enableOwnerStacks was only ever true when disableLegacyContext was already true.

]
: 'Component uses the legacy childContextTypes API which will soon be removed. Use React.createContext() instead.',
),
'Component uses the legacy contextTypes API which will soon be removed. Use React.createContext() with static contextType instead.',
]);
expect(container.firstChild.className).toBe('foo');
Expand Down
1 change: 1 addition & 0 deletions packages/shared/forks/ReactFeatureFlags.native-fb-dynamic.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,3 +26,4 @@ export const passChildrenWhenCloningPersistedNodes = __VARIANT__;
export const enableFabricCompleteRootInCommitPhase = __VARIANT__;
export const enableSiblingPrerendering = __VARIANT__;
export const enableUseResourceEffectHook = __VARIANT__;
export const enableOwnerStacks = __VARIANT__;
2 changes: 1 addition & 1 deletion packages/shared/forks/ReactFeatureFlags.native-fb.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ export const {
enableUseResourceEffectHook,
passChildrenWhenCloningPersistedNodes,
enableSiblingPrerendering,
enableOwnerStacks,
} = dynamicFlags;

// The rest of the flags are static for better dead code elimination.
Expand Down Expand Up @@ -67,7 +68,6 @@ export const enableLegacyCache = false;
export const enableLegacyFBSupport = false;
export const enableLegacyHidden = false;
export const enableNoCloningMemoCache = false;
export const enableOwnerStacks = false;
export const enablePostpone = false;
export const enableProfilerCommitHooks = __PROFILE__;
export const enableProfilerNestedUpdatePhase = __PROFILE__;
Expand Down
1 change: 1 addition & 0 deletions packages/shared/forks/ReactFeatureFlags.www-dynamic.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ export const enableRetryLaneExpiration = __VARIANT__;
export const enableTransitionTracing = __VARIANT__;
export const favorSafetyOverHydrationPerf = __VARIANT__;
export const renameElementSymbol = __VARIANT__;
export const enableOwnerStacks = __VARIANT__;
export const retryLaneExpirationMs = 5000;
export const syncLaneExpirationMs = 250;
export const transitionLaneExpirationMs = 5000;
Expand Down
2 changes: 1 addition & 1 deletion packages/shared/forks/ReactFeatureFlags.www.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ export const {
retryLaneExpirationMs,
syncLaneExpirationMs,
transitionLaneExpirationMs,
enableOwnerStacks,
} = dynamicFeatureFlags;

// On WWW, __EXPERIMENTAL__ is used for a new modern build.
Expand Down Expand Up @@ -121,7 +122,6 @@ export const useModernStrictMode = true;

export const disableLegacyMode = true;

export const enableOwnerStacks = false;
export const enableShallowPropDiffing = false;

// Flow magic to verify the exports of this file match the original version.
Expand Down
Loading