Skip to content

Warn when going to/from 0 hook #25216

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

Closed
wants to merge 4 commits into from
Closed

Warn when going to/from 0 hook #25216

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
ef8c6e6
Use dispatcheronupdate for concurrent mode
tyao1 Sep 9, 2022
9550dbd
Add a feature flag
tyao1 Sep 9, 2022
606adfe
fix test
tyao1 Sep 9, 2022
e544013
yarn lint-build
tyao1 Sep 17, 2022
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
41 changes: 31 additions & 10 deletions packages/react-reconciler/src/ReactFiberHooks.new.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ import type {HookFlags} from './ReactHookEffectTags';
import type {FiberRoot} from './ReactInternalTypes';
import type {Cache} from './ReactFiberCacheComponent.new';
import type {Flags} from './ReactFiberFlags';
import {enableThrowOnMountForHookMismatch} from 'shared/ReactFeatureFlags';

import ReactSharedInternals from 'shared/ReactSharedInternals';
import {
Expand Down Expand Up @@ -232,7 +233,6 @@ let ignorePreviousDependencies: boolean = false;
function mountHookTypesDev() {
if (__DEV__) {
const hookName = ((currentHookNameInDev: any): HookType);

if (hookTypesDev === null) {
hookTypesDev = [hookName];
} else {
Expand Down Expand Up @@ -269,7 +269,7 @@ function checkDepsAreArrayDev(deps: mixed) {
}
}

function warnOnHookMismatchInDev(currentHookName: HookType) {
function warnOnHookMismatchInDev(currentHookName: ?HookType) {
if (__DEV__) {
const componentName = getComponentNameFromFiber(currentlyRenderingFiber);
if (!didWarnAboutMismatchedHooksForComponent.has(componentName)) {
Expand All @@ -280,12 +280,18 @@ function warnOnHookMismatchInDev(currentHookName: HookType) {

const secondColumnStart = 30;

for (let i = 0; i <= ((hookTypesUpdateIndexDev: any): number); i++) {
const endIndex =
hookTypesUpdateIndexDev < 0
? hookTypesDev.length - 1
: hookTypesUpdateIndexDev;
for (let i = 0; i <= endIndex; i++) {
const oldHookName = hookTypesDev[i];
const newHookName =
i === ((hookTypesUpdateIndexDev: any): number)
(hookTypesUpdateIndexDev < 0
? null
: i === ((hookTypesUpdateIndexDev: any): number)
? currentHookName
: oldHookName;
: oldHookName) || 'undefined';

let row = `${i + 1}. ${oldHookName}`;

Expand Down Expand Up @@ -416,7 +422,12 @@ export function renderWithHooks<Props, SecondArg>(
// Non-stateful hooks (e.g. context) don't get added to memoizedState,
// so memoizedState would be null during updates and mounts.
if (__DEV__) {
if (current !== null && current.memoizedState !== null) {
if (
current !== null &&
((enableThrowOnMountForHookMismatch &&
(current.mode & ConcurrentMode) !== NoMode) ||
current.memoizedState !== null)
) {
ReactCurrentDispatcher.current = HooksDispatcherOnUpdateInDEV;
} else if (hookTypesDev !== null) {
// This dispatcher handles an edge case where a component is updating,
Expand All @@ -430,9 +441,12 @@ export function renderWithHooks<Props, SecondArg>(
}
} else {
ReactCurrentDispatcher.current =
current === null || current.memoizedState === null
? HooksDispatcherOnMount
: HooksDispatcherOnUpdate;
current !== null &&
((enableThrowOnMountForHookMismatch &&
(current.mode & ConcurrentMode) !== NoMode) ||
current.memoizedState !== null)
? HooksDispatcherOnUpdate
: HooksDispatcherOnMount;
}

let children = Component(props, secondArg);
Expand Down Expand Up @@ -485,7 +499,14 @@ export function renderWithHooks<Props, SecondArg>(
ReactCurrentDispatcher.current = ContextOnlyDispatcher;

if (__DEV__) {
workInProgress._debugHookTypes = hookTypesDev;
workInProgress._debugHookTypes = hookTypesDev || [];

if (hookTypesDev !== null) {
if (currentHookNameInDev == null && hookTypesDev.length) {
// Renders no hook from some hooks
warnOnHookMismatchInDev(null);
}
}
}

// This check uses currentHook so that it works the same in DEV and prod bundles.
Expand Down
41 changes: 31 additions & 10 deletions packages/react-reconciler/src/ReactFiberHooks.old.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ import type {HookFlags} from './ReactHookEffectTags';
import type {FiberRoot} from './ReactInternalTypes';
import type {Cache} from './ReactFiberCacheComponent.old';
import type {Flags} from './ReactFiberFlags';
import {enableThrowOnMountForHookMismatch} from 'shared/ReactFeatureFlags';

import ReactSharedInternals from 'shared/ReactSharedInternals';
import {
Expand Down Expand Up @@ -232,7 +233,6 @@ let ignorePreviousDependencies: boolean = false;
function mountHookTypesDev() {
if (__DEV__) {
const hookName = ((currentHookNameInDev: any): HookType);

if (hookTypesDev === null) {
hookTypesDev = [hookName];
} else {
Expand Down Expand Up @@ -269,7 +269,7 @@ function checkDepsAreArrayDev(deps: mixed) {
}
}

function warnOnHookMismatchInDev(currentHookName: HookType) {
function warnOnHookMismatchInDev(currentHookName: ?HookType) {
if (__DEV__) {
const componentName = getComponentNameFromFiber(currentlyRenderingFiber);
if (!didWarnAboutMismatchedHooksForComponent.has(componentName)) {
Expand All @@ -280,12 +280,18 @@ function warnOnHookMismatchInDev(currentHookName: HookType) {

const secondColumnStart = 30;

for (let i = 0; i <= ((hookTypesUpdateIndexDev: any): number); i++) {
const endIndex =
hookTypesUpdateIndexDev < 0
? hookTypesDev.length - 1
: hookTypesUpdateIndexDev;
for (let i = 0; i <= endIndex; i++) {
const oldHookName = hookTypesDev[i];
const newHookName =
i === ((hookTypesUpdateIndexDev: any): number)
(hookTypesUpdateIndexDev < 0
? null
: i === ((hookTypesUpdateIndexDev: any): number)
? currentHookName
: oldHookName;
: oldHookName) || 'undefined';

let row = `${i + 1}. ${oldHookName}`;

Expand Down Expand Up @@ -416,7 +422,12 @@ export function renderWithHooks<Props, SecondArg>(
// Non-stateful hooks (e.g. context) don't get added to memoizedState,
// so memoizedState would be null during updates and mounts.
if (__DEV__) {
if (current !== null && current.memoizedState !== null) {
if (
current !== null &&
((enableThrowOnMountForHookMismatch &&
(current.mode & ConcurrentMode) !== NoMode) ||
current.memoizedState !== null)
) {
ReactCurrentDispatcher.current = HooksDispatcherOnUpdateInDEV;
} else if (hookTypesDev !== null) {
// This dispatcher handles an edge case where a component is updating,
Expand All @@ -430,9 +441,12 @@ export function renderWithHooks<Props, SecondArg>(
}
} else {
ReactCurrentDispatcher.current =
current === null || current.memoizedState === null
? HooksDispatcherOnMount
: HooksDispatcherOnUpdate;
current !== null &&
((enableThrowOnMountForHookMismatch &&
(current.mode & ConcurrentMode) !== NoMode) ||
current.memoizedState !== null)
? HooksDispatcherOnUpdate
: HooksDispatcherOnMount;
}

let children = Component(props, secondArg);
Expand Down Expand Up @@ -485,7 +499,14 @@ export function renderWithHooks<Props, SecondArg>(
ReactCurrentDispatcher.current = ContextOnlyDispatcher;

if (__DEV__) {
workInProgress._debugHookTypes = hookTypesDev;
workInProgress._debugHookTypes = hookTypesDev || [];

if (hookTypesDev !== null) {
if (currentHookNameInDev == null && hookTypesDev.length) {
// Renders no hook from some hooks
warnOnHookMismatchInDev(null);
}
}
}

// This check uses currentHook so that it works the same in DEV and prod bundles.
Expand Down
153 changes: 152 additions & 1 deletion packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3847,6 +3847,43 @@ describe('ReactHooksWithNoopRenderer', () => {
// expect(ReactNoop.getChildren()).toEqual([span('A: 2, B: 3, C: 4')]);
});

it('mount first state', () => {
function App(props) {
let A;
if (props.loadA) {
const [_A] = useState(0);
A = _A;
} else {
A = '[not loaded]';
}

return <Text text={`A: ${A}`} />;
}

ReactNoop.render(<App loadA={false} />);
expect(Scheduler).toFlushAndYield(['A: [not loaded]']);
expect(ReactNoop.getChildren()).toEqual([span('A: [not loaded]')]);

ReactNoop.render(<App loadA={true} />);
expect(() => {
if (gate(flag => flag.enableThrowOnMountForHookMismatch)) {
expect(() => {
expect(Scheduler).toFlushAndYield(['A: 0']);
}).toThrow('Rendered more hooks than during the previous render');
} else {
expect(Scheduler).toFlushAndYield(['A: 0']);
}
}).toErrorDev([
'Warning: React has detected a change in the order of Hooks called by App. ' +
'This will lead to bugs and errors if not fixed. For more information, ' +
'read the Rules of Hooks: https://reactjs.org/link/rules-of-hooks\n\n' +
' Previous render Next render\n' +
' ------------------------------------------------------\n' +
'1. undefined useState\n' +
' ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^\n\n',
]);
});

it('unmount state', () => {
let updateA;
let updateB;
Expand Down Expand Up @@ -3887,7 +3924,121 @@ describe('ReactHooksWithNoopRenderer', () => {
);
});

it('unmount effects', () => {
it('unmount last state', () => {
function App(props) {
let A;
if (props.loadA) {
const [_A] = useState(0);
A = _A;
} else {
A = '[not loaded]';
}

return <Text text={`A: ${A}`} />;
}

ReactNoop.render(<App loadA={true} />);
expect(Scheduler).toFlushAndYield(['A: 0']);
expect(ReactNoop.getChildren()).toEqual([span('A: 0')]);
ReactNoop.render(<App loadA={false} />);
expect(() => {
expect(Scheduler).toFlushAndYield(['A: [not loaded]']);
}).toErrorDev([
'Warning: React has detected a change in the order of Hooks called by App. ' +
'This will lead to bugs and errors if not fixed. For more information, ' +
'read the Rules of Hooks: https://reactjs.org/link/rules-of-hooks\n\n' +
' Previous render Next render\n' +
' ------------------------------------------------------\n' +
'1. useState undefined\n' +
' ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^\n\n',
]);
});

it('mount effect', () => {
function App(props) {
if (props.showMore) {
useEffect(() => {
Scheduler.unstable_yieldValue('Mount A');
return () => {
Scheduler.unstable_yieldValue('Unmount A');
};
}, []);
}

return null;
}

ReactNoop.render(<App showMore={false} />);
expect(Scheduler).toFlushAndYield([]);

act(() => {
ReactNoop.render(<App showMore={true} />);
if (gate(flags => flags.enableThrowOnMountForHookMismatch)) {
expect(() => {
expect(() => {
expect(Scheduler).toFlushAndYield(['Mount A']);
}).toThrow('Rendered more hooks than during the previous render');
}).toErrorDev([
'Warning: React has detected a change in the order of Hooks called by App. ' +
'This will lead to bugs and errors if not fixed. For more information, ' +
'read the Rules of Hooks: https://reactjs.org/link/rules-of-hooks\n\n' +
' Previous render Next render\n' +
' ------------------------------------------------------\n' +
'1. undefined useEffect\n' +
' ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^\n\n',
]);
} else {
expect(() => {
expect(Scheduler).toFlushAndYield(['Mount A']);
}).toErrorDev([
'Warning: Internal React error: Expected static flag was missing. Please notify the React team.',
'Warning: React has detected a change in the order of Hooks called by App. ' +
'This will lead to bugs and errors if not fixed. For more information, ' +
'read the Rules of Hooks: https://reactjs.org/link/rules-of-hooks\n\n' +
' Previous render Next render\n' +
' ------------------------------------------------------\n' +
'1. undefined useEffect\n' +
' ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^\n\n',
]);
}
});
});

it('unmount effect', () => {
function App(props) {
if (props.showMore) {
useEffect(() => {
Scheduler.unstable_yieldValue('Mount A');
return () => {
Scheduler.unstable_yieldValue('Unmount A');
};
}, []);
}

return null;
}

ReactNoop.render(<App showMore={true} />);
expect(Scheduler).toFlushAndYield(['Mount A']);
ReactNoop.render(<App loadA={false} />);
expect(() => {
// We don't throw because it would require an additional prod check.
expect(Scheduler).not.toFlushAndThrow(
'Rendered fewer hooks than expected. This may be caused by an ' +
'accidental early return statement.',
);
}).toErrorDev([
'Warning: React has detected a change in the order of Hooks called by App. ' +
'This will lead to bugs and errors if not fixed. For more information, ' +
'read the Rules of Hooks: https://reactjs.org/link/rules-of-hooks\n\n' +
' Previous render Next render\n' +
' ------------------------------------------------------\n' +
'1. useEffect undefined\n' +
' ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^\n\n',
]);
});

it('unmount additional effects', () => {
function App(props) {
useEffect(() => {
Scheduler.unstable_yieldValue('Mount A');
Expand Down
4 changes: 4 additions & 0 deletions packages/shared/ReactFeatureFlags.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -258,3 +258,7 @@ export const enableGetInspectorDataForInstanceInProduction = false;
export const enableProfilerNestedUpdateScheduledHook = false;

export const consoleManagedByDevToolsDuringStrictMode = true;

// When going between 0 to n hooks in concurrent mode, throw "Rendered more/fewer" hooks than expected.
// This may be an invasive change so we're rolling it out slower.
export const enableThrowOnMountForHookMismatch = false;
2 changes: 2 additions & 0 deletions packages/shared/forks/ReactFeatureFlags.native-fb.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,8 @@ export const enableUseMutableSource = true;
export const enableTransitionTracing = false;

export const enableFloat = false;

export const enableThrowOnMountForHookMismatch = false;
// Flow magic to verify the exports of this file match the original version.
// eslint-disable-next-line no-unused-vars
type Check<_X, Y: _X, X: Y = _X> = null;
Expand Down
2 changes: 2 additions & 0 deletions packages/shared/forks/ReactFeatureFlags.native-oss.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,8 @@ export const enableUseMutableSource = false;
export const enableTransitionTracing = false;

export const enableFloat = false;

export const enableThrowOnMountForHookMismatch = false;
// Flow magic to verify the exports of this file match the original version.
// eslint-disable-next-line no-unused-vars
type Check<_X, Y: _X, X: Y = _X> = null;
Expand Down
2 changes: 2 additions & 0 deletions packages/shared/forks/ReactFeatureFlags.test-renderer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,8 @@ export const enableUseMutableSource = false;
export const enableTransitionTracing = false;

export const enableFloat = false;

export const enableThrowOnMountForHookMismatch = false;
// Flow magic to verify the exports of this file match the original version.
// eslint-disable-next-line no-unused-vars
type Check<_X, Y: _X, X: Y = _X> = null;
Expand Down
2 changes: 2 additions & 0 deletions packages/shared/forks/ReactFeatureFlags.test-renderer.native.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,8 @@ export const enableUseMutableSource = false;
export const enableTransitionTracing = false;

export const enableFloat = false;

export const enableThrowOnMountForHookMismatch = false;
// Flow magic to verify the exports of this file match the original version.
// eslint-disable-next-line no-unused-vars
type Check<_X, Y: _X, X: Y = _X> = null;
Expand Down
Loading