Warn if number of hooks increases

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.
facebook · Jan 16, 2019 · 6ff5571 · 6ff5571
1 parent 4c65d54
commit 6ff5571
Show file tree

Hide file tree

Showing 2 changed files with 30 additions and 18 deletions.
diff --git a/packages/react-reconciler/src/ReactFiberHooks.js b/packages/react-reconciler/src/ReactFiberHooks.js
@@ -36,6 +36,8 @@ import {
 } from './ReactFiberScheduler';
 
 import invariant from 'shared/invariant';
+import warning from 'shared/warning';
+import getComponentName from 'shared/getComponentName';
 import areHookInputsEqual from 'shared/areHookInputsEqual';
 
 type Update<S, A> = {
@@ -105,8 +107,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
@@ -148,11 +148,12 @@ export function renderWithHooks(
   // remainingExpirationTime = NoWork;
   // componentUpdateQueue = null;
 
-  // isReRender = false;
   // didScheduleRenderPhaseUpdate = false;
   // renderPhaseUpdates = null;
   // numberOfReRenders = -1;
 
+  const renderedWork: Fiber = (currentlyRenderingFiber: any);
+
   let children;
   do {
     didScheduleRenderPhaseUpdate = false;
@@ -164,13 +165,26 @@ 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;
   numberOfReRenders = -1;
 
-  const renderedWork: Fiber = (currentlyRenderingFiber: any);
-
   if (
     current !== null &&
     (renderedWork.effectTag & PerformedWork) === NoEffect
@@ -201,9 +215,6 @@ export function renderWithHooks(
   remainingExpirationTime = NoWork;
   componentUpdateQueue = null;
 
-  // Always set during createWorkInProgress
-  // isReRender = false;
-
   // These were reset above
   // didScheduleRenderPhaseUpdate = false;
   // renderPhaseUpdates = null;
@@ -237,9 +248,6 @@ export function resetHooks(): void {
   remainingExpirationTime = NoWork;
   componentUpdateQueue = null;
 
-  // Always set during createWorkInProgress
-  // isReRender = false;
-
   didScheduleRenderPhaseUpdate = false;
   renderPhaseUpdates = null;
   numberOfReRenders = -1;
@@ -273,7 +281,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
@@ -285,13 +292,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
@@ -310,7 +315,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;
     }
@@ -358,7 +362,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);

diff --git a/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.internal.js b/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.internal.js
@@ -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);
@@ -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']);