Skip to content

Commit

Permalink
Add a flag to disable double useEffect in legacy strict mode
Browse files Browse the repository at this point in the history
  • Loading branch information
@tyao1
tyao1 committed Jun 8, 2023
1 parent e1ad4aa commit 0ed49da
Show file tree
Hide file tree
Showing 13 changed files with 185 additions and 81 deletions.
12 changes: 12 additions & 0 deletions packages/react-reconciler/src/ReactFiber.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ import {
enableDebugTracing,
enableFloat,
enableHostSingletons,
enableNoDoublePassiveEffectsInLegacyStrictMode,
} from 'shared/ReactFeatureFlags';
import {NoFlags, Placement, StaticMask} from './ReactFiberFlags';
import {ConcurrentRoot} from './ReactRootTags';
Expand Down Expand Up @@ -87,6 +88,7 @@ import {
StrictLegacyMode,
StrictEffectsMode,
ConcurrentUpdatesByDefaultMode,
NoStrictPassiveEffectsMode,
} from './ReactTypeOfMode';
import {
REACT_FORWARD_REF_TYPE,
Expand Down Expand Up @@ -457,6 +459,9 @@ export function createHostRootFiber(
mode = ConcurrentMode;
if (isStrictMode === true || createRootStrictEffectsByDefault) {
mode |= StrictLegacyMode | StrictEffectsMode;
if (__DEV__ && enableNoDoublePassiveEffectsInLegacyStrictMode) {
mode |= NoStrictPassiveEffectsMode;
}
}
if (
// We only use this flag for our repo tests to check both behaviors.
Expand Down Expand Up @@ -539,6 +544,9 @@ export function createFiberFromTypeAndProps(
if ((mode & ConcurrentMode) !== NoMode) {
// Strict effects should never run on legacy roots
mode |= StrictEffectsMode;
if (__DEV__ && enableNoDoublePassiveEffectsInLegacyStrictMode) {
mode |= NoStrictPassiveEffectsMode;
}
}
break;
case REACT_PROFILER_TYPE:
Expand Down Expand Up @@ -752,6 +760,10 @@ export function createFiberFromOffscreen(
lanes: Lanes,
key: null | string,
): Fiber {
if (__DEV__) {
// Offscreen should always run double passive effects
mode &= ~NoStrictPassiveEffectsMode;
}
const fiber = createFiber(OffscreenComponent, pendingProps, key, mode);
fiber.elementType = REACT_OFFSCREEN_TYPE;
fiber.lanes = lanes;
Expand Down
4 changes: 3 additions & 1 deletion packages/react-reconciler/src/ReactFiberHooks.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,7 @@ import {
DebugTracingMode,
StrictEffectsMode,
StrictLegacyMode,
NoStrictPassiveEffectsMode,
} from './ReactTypeOfMode';
import {
NoLane,
Expand Down Expand Up @@ -2257,7 +2258,8 @@ function mountEffect(
): void {
if (
__DEV__ &&
(currentlyRenderingFiber.mode & StrictEffectsMode) !== NoMode
(currentlyRenderingFiber.mode & StrictEffectsMode) !== NoMode &&
(currentlyRenderingFiber.mode & NoStrictPassiveEffectsMode) === NoMode
) {
mountEffectImpl(
MountPassiveDevEffect | PassiveEffect | PassiveStaticEffect,
Expand Down
15 changes: 8 additions & 7 deletions packages/react-reconciler/src/ReactTypeOfMode.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,11 +9,12 @@

export type TypeOfMode = number;

export const NoMode = /* */ 0b000000;
export const NoMode = /* */ 0b0000000;
// TODO: Remove ConcurrentMode by reading from the root tag instead
export const ConcurrentMode = /* */ 0b000001;
export const ProfileMode = /* */ 0b000010;
export const DebugTracingMode = /* */ 0b000100;
export const StrictLegacyMode = /* */ 0b001000;
export const StrictEffectsMode = /* */ 0b010000;
export const ConcurrentUpdatesByDefaultMode = /* */ 0b100000;
export const ConcurrentMode = /* */ 0b0000001;
export const ProfileMode = /* */ 0b0000010;
export const DebugTracingMode = /* */ 0b0000100;
export const StrictLegacyMode = /* */ 0b0001000;
export const StrictEffectsMode = /* */ 0b0010000;
export const ConcurrentUpdatesByDefaultMode = /* */ 0b0100000;
export const NoStrictPassiveEffectsMode = /* */ 0b1000000;
163 changes: 110 additions & 53 deletions packages/react-reconciler/src/__tests__/StrictEffectsModeDefaults-test.internal.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -134,6 +134,7 @@ describe('StrictEffectsMode defaults', () => {

// This test also verifies that double-invoked effects flush synchronously
// within the same frame as passive effects.
// @gate !enableNoDoublePassiveEffectsInLegacyStrictMode
it('should double invoke effects only for newly mounted components', async () => {
function ComponentWithEffects({label}) {
React.useEffect(() => {
Expand Down Expand Up @@ -212,14 +213,23 @@ describe('StrictEffectsMode defaults', () => {
ReactNoop.render(<App text={'mount'} />);
});

assertLog([
'useLayoutEffect mount',
'useEffect mount',
'useLayoutEffect unmount',
'useEffect unmount',
'useLayoutEffect mount',
'useEffect mount',
]);
assertLog(
gate(flags => flags.enableNoDoublePassiveEffectsInLegacyStrictMode)
? [
'useLayoutEffect mount',
'useEffect mount',
'useLayoutEffect unmount',
'useLayoutEffect mount',
]
: [
'useLayoutEffect mount',
'useEffect mount',
'useLayoutEffect unmount',
'useEffect unmount',
'useLayoutEffect mount',
'useEffect mount',
],
);

await act(() => {
ReactNoop.render(<App text={'update'} />);
Expand Down Expand Up @@ -258,14 +268,18 @@ describe('StrictEffectsMode defaults', () => {
ReactNoop.render(<App text={'mount'} />);
});

assertLog([
'useEffect One mount',
'useEffect Two mount',
'useEffect One unmount',
'useEffect Two unmount',
'useEffect One mount',
'useEffect Two mount',
]);
assertLog(
gate(flags => flags.enableNoDoublePassiveEffectsInLegacyStrictMode)
? ['useEffect One mount', 'useEffect Two mount']
: [
'useEffect One mount',
'useEffect Two mount',
'useEffect One unmount',
'useEffect Two unmount',
'useEffect One mount',
'useEffect Two mount',
],
);

await act(() => {
ReactNoop.render(<App text={'update'} />);
Expand Down Expand Up @@ -348,12 +362,20 @@ describe('StrictEffectsMode defaults', () => {
ReactNoop.render(<App text={'mount'} />);
});

assertLog([
'useLayoutEffect mount',
'useEffect mount',
'useLayoutEffect mount',
'useEffect mount',
]);
assertLog(
gate(flags => flags.enableNoDoublePassiveEffectsInLegacyStrictMode)
? [
'useLayoutEffect mount',
'useEffect mount',
'useLayoutEffect mount',
]
: [
'useLayoutEffect mount',
'useEffect mount',
'useLayoutEffect mount',
'useEffect mount',
],
);

await act(() => {
ReactNoop.render(<App text={'update'} />);
Expand Down Expand Up @@ -469,6 +491,7 @@ describe('StrictEffectsMode defaults', () => {
assertLog(['componentWillUnmount']);
});

// @gate !enableNoDoublePassiveEffectsInLegacyStrictMode
it('double flushing passive effects only results in one double invoke', async () => {
function App({text}) {
const [state, setState] = React.useState(0);
Expand Down Expand Up @@ -543,31 +566,53 @@ describe('StrictEffectsMode defaults', () => {
ReactNoop.render(<App />);
});

assertLog([
'App useLayoutEffect mount',
'App useEffect mount',
'App useLayoutEffect unmount',
'App useEffect unmount',
'App useLayoutEffect mount',
'App useEffect mount',
]);
assertLog(
gate(flags => flags.enableNoDoublePassiveEffectsInLegacyStrictMode)
? [
'App useLayoutEffect mount',
'App useEffect mount',
'App useLayoutEffect unmount',
'App useLayoutEffect mount',
]
: [
'App useLayoutEffect mount',
'App useEffect mount',
'App useLayoutEffect unmount',
'App useEffect unmount',
'App useLayoutEffect mount',
'App useEffect mount',
],
);

await act(() => {
_setShowChild(true);
});

assertLog([
'App useLayoutEffect unmount',
'Child useLayoutEffect mount',
'App useLayoutEffect mount',
'App useEffect unmount',
'Child useEffect mount',
'App useEffect mount',
'Child useLayoutEffect unmount',
'Child useEffect unmount',
'Child useLayoutEffect mount',
'Child useEffect mount',
]);
assertLog(
gate(flags => flags.enableNoDoublePassiveEffectsInLegacyStrictMode)
? [
'App useLayoutEffect unmount',
'Child useLayoutEffect mount',
'App useLayoutEffect mount',
'App useEffect unmount',
'Child useEffect mount',
'App useEffect mount',
'Child useLayoutEffect unmount',
'Child useLayoutEffect mount',
]
: [
'App useLayoutEffect unmount',
'Child useLayoutEffect mount',
'App useLayoutEffect mount',
'App useEffect unmount',
'Child useEffect mount',
'App useEffect mount',
'Child useLayoutEffect unmount',
'Child useEffect unmount',
'Child useLayoutEffect mount',
'Child useEffect mount',
],
);
});

it('classes and functions are double invoked together correctly', async () => {
Expand Down Expand Up @@ -610,17 +655,29 @@ describe('StrictEffectsMode defaults', () => {
ReactNoop.render(<App text={'mount'} />);
});

assertLog([
'componentDidMount',
'useLayoutEffect mount',
'useEffect mount',
'componentWillUnmount',
'useLayoutEffect unmount',
'useEffect unmount',
'componentDidMount',
'useLayoutEffect mount',
'useEffect mount',
]);
assertLog(
gate(flags => flags.enableNoDoublePassiveEffectsInLegacyStrictMode)
? [
'componentDidMount',
'useLayoutEffect mount',
'useEffect mount',
'componentWillUnmount',
'useLayoutEffect unmount',
'componentDidMount',
'useLayoutEffect mount',
]
: [
'componentDidMount',
'useLayoutEffect mount',
'useEffect mount',
'componentWillUnmount',
'useLayoutEffect unmount',
'useEffect unmount',
'componentDidMount',
'useLayoutEffect mount',
'useEffect mount',
],
);

await act(() => {
ReactNoop.render(<App text={'mount'} />);
Expand Down
63 changes: 43 additions & 20 deletions packages/react/src/__tests__/ReactStrictMode-test.internal.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -69,16 +69,27 @@ describe('ReactStrictMode', () => {
root.render(<Component label="A" />);
});

expect(log).toEqual([
'A: render',
'A: render',
'A: useLayoutEffect mount',
'A: useEffect mount',
'A: useLayoutEffect unmount',
'A: useEffect unmount',
'A: useLayoutEffect mount',
'A: useEffect mount',
]);
expect(log).toEqual(
gate(flags => flags.enableNoDoublePassiveEffectsInLegacyStrictMode)
? [
'A: render',
'A: render',
'A: useLayoutEffect mount',
'A: useEffect mount',
'A: useLayoutEffect unmount',
'A: useLayoutEffect mount',
]
: [
'A: render',
'A: render',
'A: useLayoutEffect mount',
'A: useEffect mount',
'A: useLayoutEffect unmount',
'A: useEffect unmount',
'A: useLayoutEffect mount',
'A: useEffect mount',
],
);
});

it('should include legacy + strict effects mode', async () => {
Expand All @@ -92,18 +103,30 @@ describe('ReactStrictMode', () => {
);
});

expect(log).toEqual([
'A: render',
'A: render',
'A: useLayoutEffect mount',
'A: useEffect mount',
'A: useLayoutEffect unmount',
'A: useEffect unmount',
'A: useLayoutEffect mount',
'A: useEffect mount',
]);
expect(log).toEqual(
gate(flags => flags.enableNoDoublePassiveEffectsInLegacyStrictMode)
? [
'A: render',
'A: render',
'A: useLayoutEffect mount',
'A: useEffect mount',
'A: useLayoutEffect unmount',
'A: useLayoutEffect mount',
]
: [
'A: render',
'A: render',
'A: useLayoutEffect mount',
'A: useEffect mount',
'A: useLayoutEffect unmount',
'A: useEffect unmount',
'A: useLayoutEffect mount',
'A: useEffect mount',
],
);
});

// @gate !enableNoDoublePassiveEffectsInLegacyStrictMode
it('should allow level to be increased with nesting', async () => {
await act(() => {
const container = document.createElement('div');
Expand Down
2 changes: 2 additions & 0 deletions packages/shared/ReactFeatureFlags.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -237,3 +237,5 @@ export const consoleManagedByDevToolsDuringStrictMode = true;
// components will encounter in production, especially when used With <Offscreen />.
// TODO: clean up legacy <StrictMode /> once tests pass WWW.
export const useModernStrictMode = false;

export const enableNoDoublePassiveEffectsInLegacyStrictMode = false;
1 change: 1 addition & 0 deletions packages/shared/forks/ReactFeatureFlags.native-fb.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,7 @@ export const enableFloat = true;
export const enableHostSingletons = true;

export const useModernStrictMode = false;
export const enableNoDoublePassiveEffectsInLegacyStrictMode = false;
export const enableFizzExternalRuntime = false;

export const diffInCommitPhase = true;
Expand Down
Loading

0 comments on commit 0ed49da

Please sign in to comment.