Skip to content

Commit

Permalink
Warn if number of hooks increases (facebook#14591)
Browse files Browse the repository at this point in the history
Eventually, we'll likely support adding hooks to the end (to enable
progressive enhancement), but let's warn until we figure out how it
should work.
  • Loading branch information
acdlite authored and n8schloss committed Jan 31, 2019
1 parent 45a61e6 commit e40d317
Show file tree
Hide file tree
Showing 2 changed files with 28 additions and 16 deletions.
32 changes: 18 additions & 14 deletions packages/react-reconciler/src/ReactFiberHooks.js
Expand Up @@ -34,6 +34,8 @@ import {
} from './ReactFiberScheduler';

import invariant from 'shared/invariant';
import warning from 'shared/warning';
import getComponentName from 'shared/getComponentName';
import areHookInputsEqual from 'shared/areHookInputsEqual';
import {markWorkInProgressReceivedUpdate} from './ReactFiberBeginWork';

Expand Down Expand Up @@ -104,8 +106,6 @@ let componentUpdateQueue: FunctionComponentUpdateQueue | null = null;
// map of queue -> render-phase updates, which are discarded once the component
// completes without re-rendering.

// Whether the work-in-progress hook is a re-rendered hook
let isReRender: boolean = false;
// Whether an update was scheduled during the currently executing render pass.
let didScheduleRenderPhaseUpdate: boolean = false;
// Lazily created map of render-phase updates
Expand Down Expand Up @@ -147,7 +147,6 @@ export function renderWithHooks(
// remainingExpirationTime = NoWork;
// componentUpdateQueue = null;

// isReRender = false;
// didScheduleRenderPhaseUpdate = false;
// renderPhaseUpdates = null;
// numberOfReRenders = -1;
Expand All @@ -163,6 +162,21 @@ export function renderWithHooks(
componentUpdateQueue = null;

children = Component(props, refOrContext);

if (__DEV__) {
if (
current !== null &&
workInProgressHook !== null &&
currentHook === null
) {
warning(
false,
'%s: Rendered more hooks than during the previous render. This is ' +
'not currently supported and may lead to unexpected behavior.',
getComponentName(Component),
);
}
}
} while (didScheduleRenderPhaseUpdate);

renderPhaseUpdates = null;
Expand All @@ -188,9 +202,6 @@ export function renderWithHooks(
remainingExpirationTime = NoWork;
componentUpdateQueue = null;

// Always set during createWorkInProgress
// isReRender = false;

// These were reset above
// didScheduleRenderPhaseUpdate = false;
// renderPhaseUpdates = null;
Expand Down Expand Up @@ -236,9 +247,6 @@ export function resetHooks(): void {
remainingExpirationTime = NoWork;
componentUpdateQueue = null;

// Always set during createWorkInProgress
// isReRender = false;

didScheduleRenderPhaseUpdate = false;
renderPhaseUpdates = null;
numberOfReRenders = -1;
Expand Down Expand Up @@ -272,7 +280,6 @@ function createWorkInProgressHook(): Hook {
if (workInProgressHook === null) {
// This is the first hook in the list
if (firstWorkInProgressHook === null) {
isReRender = false;
currentHook = firstCurrentHook;
if (currentHook === null) {
// This is a newly mounted hook
Expand All @@ -284,13 +291,11 @@ function createWorkInProgressHook(): Hook {
firstWorkInProgressHook = workInProgressHook;
} else {
// There's already a work-in-progress. Reuse it.
isReRender = true;
currentHook = firstCurrentHook;
workInProgressHook = firstWorkInProgressHook;
}
} else {
if (workInProgressHook.next === null) {
isReRender = false;
let hook;
if (currentHook === null) {
// This is a newly mounted hook
Expand All @@ -309,7 +314,6 @@ function createWorkInProgressHook(): Hook {
workInProgressHook = workInProgressHook.next = hook;
} else {
// There's already a work-in-progress. Reuse it.
isReRender = true;
workInProgressHook = workInProgressHook.next;
currentHook = currentHook !== null ? currentHook.next : null;
}
Expand Down Expand Up @@ -357,7 +361,7 @@ export function useReducer<S, A>(
let queue: UpdateQueue<S, A> | null = (workInProgressHook.queue: any);
if (queue !== null) {
// Already have a queue, so this is an update.
if (isReRender) {
if (numberOfReRenders > 0) {
// This is a re-render. Apply the new render phase updates to the previous
// work-in-progress hook.
const dispatch: Dispatch<A> = (queue.dispatch: any);
Expand Down
Expand Up @@ -1634,7 +1634,11 @@ describe('ReactHooksWithNoopRenderer', () => {
]);

ReactNoop.render(<App loadC={true} />);
expect(ReactNoop.flush()).toEqual(['A: 2, B: 3, C: 0']);
expect(() => {
expect(ReactNoop.flush()).toEqual(['A: 2, B: 3, C: 0']);
}).toWarnDev([
'App: Rendered more hooks than during the previous render',
]);
expect(ReactNoop.getChildren()).toEqual([span('A: 2, B: 3, C: 0')]);

updateC(4);
Expand Down Expand Up @@ -1708,7 +1712,11 @@ describe('ReactHooksWithNoopRenderer', () => {
expect(ReactNoop.clearYields()).toEqual(['Mount A']);

ReactNoop.render(<App showMore={true} />);
expect(ReactNoop.flush()).toEqual([]);
expect(() => {
expect(ReactNoop.flush()).toEqual([]);
}).toWarnDev([
'App: Rendered more hooks than during the previous render',
]);
flushPassiveEffects();
expect(ReactNoop.clearYields()).toEqual(['Mount B']);

Expand Down

0 comments on commit e40d317

Please sign in to comment.