Warn about variable number of dependencies

acdlite · acdlite · commit e02086bfcc69 · 2018-10-29T13:51:43.000-07:00
We don't check this in prod, since best practice is to always pass
these inline. But we should still warn in dev.
diff --git a/packages/react-reconciler/src/ReactFiberHooks.js b/packages/react-reconciler/src/ReactFiberHooks.js
@@ -37,6 +37,7 @@ import {
 } from './ReactFiberScheduler';
 
 import invariant from 'shared/invariant';
+import warning from 'shared/warning';
 import warningWithoutStack from 'shared/warningWithoutStack';
 import {enableDispatchCallback_DEPRECATED} from 'shared/ReactFeatureFlags';
 
@@ -760,8 +761,19 @@ function dispatchAction<S, A>(
 }
 
 function inputsAreEqual(arr1, arr2) {
-  // Don't bother comparing lengths because these arrays are always
+  // Don't bother comparing lengths in prod because these arrays should be
   // passed inline.
+  if (__DEV__) {
+    warning(
+      arr1.length === arr2.length,
+      'Detected a variable number of hook dependencies. The length of the ' +
+        'dependencies array should be constant between renders.\n\n' +
+        'Previous: %s\n' +
+        'Incoming: %s',
+      arr1.join(', '),
+      arr2.join(', '),
+    );
+  }
   for (let i = 0; i < arr1.length; i++) {
     // Inlined Object.is polyfill.
     // https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/is
diff --git a/packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js b/packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js
@@ -0,0 +1,54 @@
+/**
+ * Copyright (c) 2013-present, Facebook, Inc.
+ *
+ * This source code is licensed under the MIT license found in the
+ * LICENSE file in the root directory of this source tree.
+ *
+ * @emails react-core
+ * @jest-environment node
+ */
+
+/* eslint-disable no-func-assign */
+
+'use strict';
+
+let React;
+let ReactFeatureFlags;
+let ReactTestRenderer;
+
+// Additional tests can be found in ReactHooksWithNoopRenderer. Plan is to
+// gradually migrate those to this file.
+describe('ReactHooks', () => {
+  beforeEach(() => {
+    jest.resetModules();
+
+    ReactFeatureFlags = require('shared/ReactFeatureFlags');
+    ReactFeatureFlags.debugRenderPhaseSideEffectsForStrictMode = false;
+    ReactFeatureFlags.enableHooks = true;
+    React = require('react');
+    ReactTestRenderer = require('react-test-renderer');
+  });
+
+  it('warns about variable number of dependencies', () => {
+    const {useLayoutEffect} = React;
+    function App(props) {
+      useLayoutEffect(() => {
+        ReactTestRenderer.unstable_yield(
+          'Did commit: ' + props.dependencies.join(', '),
+        );
+      }, props.dependencies);
+      return props.dependencies;
+    }
+    const root = ReactTestRenderer.create(<App dependencies={['A']} />);
+    expect(ReactTestRenderer).toHaveYielded(['Did commit: A']);
+    expect(() => {
+      root.update(<App dependencies={['A', 'B']} />);
+    }).toWarnDev([
+      'Warning: Detected a variable number of hook dependencies. The length ' +
+        'of the dependencies array should be constant between renders.\n\n' +
+        'Previous: A, B\n' +
+        'Incoming: A',
+    ]);
+    expect(ReactTestRenderer).toHaveYielded(['Did commit: A, B']);
+  });
+});
diff --git a/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.internal.js b/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.internal.js
@@ -29,7 +29,10 @@ let forwardRef;
 let flushPassiveEffects;
 let memo;
 
-describe('ReactHooks', () => {
+// These tests use React Noop Renderer. All new tests should use React Test
+// Renderer and go in ReactHooks-test; plan is gradually migrate the noop tests
+// to that file.
+describe('ReactHooksWithNoopRenderer', () => {
   beforeEach(() => {
     jest.resetModules();
 
