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.

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

DEV warning for "strict effects mode" (Facebook only) #20694

Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
40 changes: 40 additions & 0 deletions packages/react-reconciler/src/ReactFiberWorkLoop.new.js
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,8 @@ import {
disableSchedulerTimeoutInWorkLoop,
enableDoubleInvokingEffects,
skipUnmountedBoundaries,
enableStrictEffectsModeDevWarningForFacebookOnly,
bypassStrictEffectsModeDevWarningForFacebookOnly,
} from 'shared/ReactFeatureFlags';
import ReactSharedInternals from 'shared/ReactSharedInternals';
import invariant from 'shared/invariant';
Expand Down Expand Up @@ -2578,6 +2580,8 @@ function flushRenderPhaseStrictModeWarningsInDEV() {
}
}

let didWarnAboutStrictEffectsMode: boolean = false;

function commitDoubleInvokeEffectsInDEV(
fiber: Fiber,
hasPassiveEffects: boolean,
Expand All @@ -2588,6 +2592,42 @@ function commitDoubleInvokeEffectsInDEV(
return;
}

if (
__DEV__ &&
enableStrictEffectsModeDevWarningForFacebookOnly &&
!bypassStrictEffectsModeDevWarningForFacebookOnly
) {
if (!didWarnAboutStrictEffectsMode) {
didWarnAboutStrictEffectsMode = true;

// While we roll out strict effects mode within Facebook,
// let's provide a little more context about what it is and why it exists.
// This is primarily intended for people who did not read the internal announcement.
//
// Note we don't check subtreeFlags here to verify there actually are effects
// because it's rare enough for there not to be that it's unnecessary.
//
// The warning includes Facebook specific instructions for how to disable it
// (using a GK blocklist). If we decide to add a similar warning for OSS we'll need
// to revisit both the wording and the opt-out mechansim.
//
// eslint-disable-next-line react-internal/no-production-logging
console.warn(
'React strict effects mode is enabled for this application. ' +
'In this mode, React will run effects twice after a component mounts ' +
'(create -> destroy -> recreate) to simulate the component being unmounted ' +
'and then remounted as may happen with future React APIs like Offscreen. ' +
'Mount-only effects (ones with an empty dependencies array) should be ' +
'resilient to being run more than once.' +
'\n\nTo disable this warning, add yourself to the blocklist for the' +
'react_enable_strict_effects_mode_warning GK here: ' +
'https://fburl.com/react-disable-strict-effects-mode-warning' +
'\n\nFor more information about strict effects mode, see ' +
'https://fburl.com/react-strict-effects-mode',
);
}
}

setCurrentDebugFiberInDEV(fiber);
invokeEffectsInDev(fiber, MountLayoutDev, invokeLayoutEffectUnmountInDEV);
if (hasPassiveEffects) {
Expand Down
40 changes: 40 additions & 0 deletions packages/react-reconciler/src/ReactFiberWorkLoop.old.js
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,8 @@ import {
enableDoubleInvokingEffects,
skipUnmountedBoundaries,
enableDiscreteEventMicroTasks,
enableStrictEffectsModeDevWarningForFacebookOnly,
bypassStrictEffectsModeDevWarningForFacebookOnly,
} from 'shared/ReactFeatureFlags';
import ReactSharedInternals from 'shared/ReactSharedInternals';
import invariant from 'shared/invariant';
Expand Down Expand Up @@ -2558,6 +2560,8 @@ function flushRenderPhaseStrictModeWarningsInDEV() {
}
}

let didWarnAboutStrictEffectsMode: boolean = false;

function commitDoubleInvokeEffectsInDEV(
fiber: Fiber,
hasPassiveEffects: boolean,
Expand All @@ -2568,6 +2572,42 @@ function commitDoubleInvokeEffectsInDEV(
return;
}

if (
__DEV__ &&
enableStrictEffectsModeDevWarningForFacebookOnly &&
!bypassStrictEffectsModeDevWarningForFacebookOnly
) {
if (!didWarnAboutStrictEffectsMode) {
didWarnAboutStrictEffectsMode = true;

// While we roll out strict effects mode within Facebook,
// let's provide a little more context about what it is and why it exists.
// This is primarily intended for people who did not read the internal announcement.
//
// Note we don't check subtreeFlags here to verify there actually are effects
// because it's rare enough for there not to be that it's unnecessary.
//
// The warning includes Facebook specific instructions for how to disable it
// (using a GK blocklist). If we decide to add a similar warning for OSS we'll need
// to revisit both the wording and the opt-out mechansim.
//
// eslint-disable-next-line react-internal/no-production-logging
console.warn(
'React strict effects mode is enabled for this application. ' +
'In this mode, React will run effects twice after a component mounts ' +
'(create -> destroy -> recreate) to simulate the component being unmounted ' +
'and then remounted as may happen with future React APIs like Offscreen. ' +
'Mount-only effects (ones with an empty dependencies array) should be ' +
'resilient to being run more than once.' +
'\n\nTo disable this warning, add yourself to the blocklist for the' +
'react_enable_strict_effects_mode_warning GK here: ' +
'https://fburl.com/react-disable-strict-effects-mode-warning' +
'\n\nFor more information about strict effects mode, see ' +
'https://fburl.com/react-strict-effects-mode',
);
}
}

setCurrentDebugFiberInDEV(fiber);
invokeEffectsInDev(fiber, MountLayoutDev, invokeLayoutEffectUnmountInDEV);
if (hasPassiveEffects) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,77 @@ describe('ReactDoubleInvokeEvents', () => {
ReactFeatureFlags.enableDoubleInvokingEffects = shouldDoubleInvokingEffects();
});

describe('Facebook strict effects mode warning', () => {
beforeEach(() => {
ReactFeatureFlags.enableStrictEffectsModeDevWarningForFacebookOnly = true;
ReactFeatureFlags.bypassStrictEffectsModeDevWarningForFacebookOnly = false;
});

it('should log before double invoking effects the first time', () => {
function Component() {
React.useLayoutEffect(() => {}, []);
return null;
}

expect(() => {
ReactNoop.act(() => {
ReactNoop.render(
<>
<Component key="1" />
</>,
);
});
}).toWarnDev(
'React strict effects mode is enabled for this application.',
{
withoutStack: true,
},
);

// But not on updates, even if new components have mounted.
ReactNoop.act(() => {
ReactNoop.render(
<>
<Component key="1" />
<Component key="2" />
</>,
);
});
});

it('should not log for legacy roots even if strict mode is enabled', () => {
function Component() {
React.useLayoutEffect(() => {}, []);
return null;
}

ReactNoop.act(() => {
ReactNoop.renderLegacySyncRoot(
<React.StrictMode>
<Component />
</React.StrictMode>,
);
});
});

it('should not log if bypass flag has been enabled (by GK allowlist)', () => {
ReactFeatureFlags.bypassStrictEffectsModeDevWarningForFacebookOnly = true;

function Component() {
React.useLayoutEffect(() => {}, []);
return null;
}

ReactNoop.act(() => {
ReactNoop.render(
<React.StrictMode>
<Component />
</React.StrictMode>,
);
});
});
});

it('should not double invoke effects in legacy mode', () => {
function App({text}) {
React.useEffect(() => {
Expand Down
15 changes: 15 additions & 0 deletions packages/shared/ReactFeatureFlags.js
Original file line number Diff line number Diff line change
Expand Up @@ -154,3 +154,18 @@ export const disableSchedulerTimeoutInWorkLoop = false;
export const enableTransitionEntanglement = false;

export const enableDiscreteEventMicroTasks = false;

// This flag controls a temporary DEV mode warning that explains strict effects mode.
// This flag should only be enabled for Facebook builds, because the warning includes
// Facebook specific instructions for how to disable it (using a GK blocklist).
// If we decide to add a similar warning for OSS we'll need to revisit both the
// wording and the opt-out mechansim.
//
// This warning is controlled by two flags so that (a) we can enable it for 100%
// of Facebook engineers¹² only and (b) provide a simple opt-out mechanism for engineers
// who know about the mode and no longer want to see the warning.
//
// ¹ This warning will not be logged for legacy applications that aren't in strict effects mode.
// ² GKs within Facebook can't be enabled for 100% of a population.
export const enableStrictEffectsModeDevWarningForFacebookOnly = false;
export const bypassStrictEffectsModeDevWarningForFacebookOnly = false;
2 changes: 2 additions & 0 deletions packages/shared/forks/ReactFeatureFlags.native-fb.js
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,8 @@ export const enableRecursiveCommitTraversal = false;
export const disableSchedulerTimeoutInWorkLoop = false;
export const enableTransitionEntanglement = false;
export const enableDiscreteEventMicroTasks = false;
export const enableStrictEffectsModeDevWarningForFacebookOnly = false;
export const bypassStrictEffectsModeDevWarningForFacebookOnly = false;

// Flow magic to verify the exports of this file match the original version.
// eslint-disable-next-line no-unused-vars
Expand Down
2 changes: 2 additions & 0 deletions packages/shared/forks/ReactFeatureFlags.native-oss.js
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,8 @@ export const enableRecursiveCommitTraversal = false;
export const disableSchedulerTimeoutInWorkLoop = false;
export const enableTransitionEntanglement = false;
export const enableDiscreteEventMicroTasks = false;
export const enableStrictEffectsModeDevWarningForFacebookOnly = false;
export const bypassStrictEffectsModeDevWarningForFacebookOnly = false;

// Flow magic to verify the exports of this file match the original version.
// eslint-disable-next-line no-unused-vars
Expand Down
2 changes: 2 additions & 0 deletions packages/shared/forks/ReactFeatureFlags.test-renderer.js
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,8 @@ export const enableRecursiveCommitTraversal = false;
export const disableSchedulerTimeoutInWorkLoop = false;
export const enableTransitionEntanglement = false;
export const enableDiscreteEventMicroTasks = false;
export const enableStrictEffectsModeDevWarningForFacebookOnly = false;
export const bypassStrictEffectsModeDevWarningForFacebookOnly = false;

// Flow magic to verify the exports of this file match the original version.
// eslint-disable-next-line no-unused-vars
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,8 @@ export const enableLegacyFBSupport = false;
export const enableFilterEmptyStringAttributesDOM = false;
export const disableNativeComponentFrames = false;
export const skipUnmountedBoundaries = false;
export const enableStrictEffectsModeDevWarningForFacebookOnly = false;
export const bypassStrictEffectsModeDevWarningForFacebookOnly = false;

export const enableNewReconciler = false;
export const deferRenderPhaseUpdateToNextBatch = true;
Expand Down
2 changes: 2 additions & 0 deletions packages/shared/forks/ReactFeatureFlags.test-renderer.www.js
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,8 @@ export const enableRecursiveCommitTraversal = false;
export const disableSchedulerTimeoutInWorkLoop = false;
export const enableTransitionEntanglement = false;
export const enableDiscreteEventMicroTasks = false;
export const enableStrictEffectsModeDevWarningForFacebookOnly = false;
export const bypassStrictEffectsModeDevWarningForFacebookOnly = false;

// Flow magic to verify the exports of this file match the original version.
// eslint-disable-next-line no-unused-vars
Expand Down
2 changes: 2 additions & 0 deletions packages/shared/forks/ReactFeatureFlags.testing.js
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,8 @@ export const enableRecursiveCommitTraversal = false;
export const disableSchedulerTimeoutInWorkLoop = false;
export const enableTransitionEntanglement = false;
export const enableDiscreteEventMicroTasks = false;
export const enableStrictEffectsModeDevWarningForFacebookOnly = false;
export const bypassStrictEffectsModeDevWarningForFacebookOnly = false;

// Flow magic to verify the exports of this file match the original version.
// eslint-disable-next-line no-unused-vars
Expand Down
2 changes: 2 additions & 0 deletions packages/shared/forks/ReactFeatureFlags.testing.www.js
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,8 @@ export const enableRecursiveCommitTraversal = false;
export const disableSchedulerTimeoutInWorkLoop = false;
export const enableTransitionEntanglement = false;
export const enableDiscreteEventMicroTasks = false;
export const enableStrictEffectsModeDevWarningForFacebookOnly = false;
export const bypassStrictEffectsModeDevWarningForFacebookOnly = false;

// Flow magic to verify the exports of this file match the original version.
// eslint-disable-next-line no-unused-vars
Expand Down
5 changes: 5 additions & 0 deletions packages/shared/forks/ReactFeatureFlags.www-dynamic.js
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,11 @@ export const enableLegacyFBSupport = __VARIANT__;
export const decoupleUpdatePriorityFromScheduler = __VARIANT__;
export const skipUnmountedBoundaries = __VARIANT__;

// enableStrictEffectsModeDevWarningForFacebookOnly is true for all Facebook builds by default
// because a GK shouldn't be used to roll a feature to 100% of employees.
// So this flag is disabled by default in the dynamic file to avoid spamming tests.
export const bypassStrictEffectsModeDevWarningForFacebookOnly = true;

// Enable this flag to help with concurrent mode debugging.
// It logs information to the console about React scheduling, rendering, and commit phases.
//
Expand Down
3 changes: 3 additions & 0 deletions packages/shared/forks/ReactFeatureFlags.www.js
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ export const {
disableSchedulerTimeoutInWorkLoop,
enableTransitionEntanglement,
enableDiscreteEventMicroTasks,
bypassStrictEffectsModeDevWarningForFacebookOnly,
} = dynamicFeatureFlags;

// On WWW, __EXPERIMENTAL__ is used for a new modern build.
Expand Down Expand Up @@ -87,6 +88,8 @@ export const warnUnstableRenderSubtreeIntoContainer = false;

export const enableDiscreteEventFlushingChange = true;

export const enableStrictEffectsModeDevWarningForFacebookOnly = __DEV__;

// Enable forked reconciler. Piggy-backing on the "variant" global so that we
// don't have to add another test dimension. The build system will compile this
// to the correct value.
Expand Down