Skip to content

Commit

Permalink
Warn about variable number of dependencies
Browse files Browse the repository at this point in the history
We don't check this in prod, since best practice is to always pass
these inline. But we should still warn in dev.
  • Loading branch information
acdlite committed Oct 29, 2018
1 parent b92cdef commit e02086b
Show file tree
Hide file tree
Showing 3 changed files with 71 additions and 2 deletions.
14 changes: 13 additions & 1 deletion packages/react-reconciler/src/ReactFiberHooks.js
Original file line number Diff line number Diff line change
Expand Up @@ -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';

Expand Down Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
@@ -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']);
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -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();

Expand Down

0 comments on commit e02086b

Please sign in to comment.