Don't bother comparing constructor when deps are not provided

When no dependencies are passed to an effect hook, what we used to do is
compare the effect constructor. If there was no change, then we would
skip firing the effect. In practice, this is a useless optimization
because the constructor will always be different when you pass an inline
closure. And if you don't pass an inline closure, then you can't access
any props or state.

There are some edge cases where an effect that doesn't close over props
or state could be useful, like reference counting the number of mounted
components. But those are rare and can be addressed by passing an empty
array of dependencies.

By removing this "optimization," we can avoid retaining the constructor
in the majority of cases where it's a closure that changes on
every render.

I made corresponding changes to the other hooks that accept
dependencies, too (useMemo, useCallback, and useImperativeHandle).

packages/react-reconciler/src/ReactFiberHooks.js

@@ -61,7 +61,7 @@ type Effect = {
   tag: HookEffectTag,
   create: () => mixed,
   destroy: (() => mixed) | null,
-  inputs: Array<mixed>,
+  deps: Array<mixed> | null,
   next: Effect,
 };
 
@@ -471,12 +471,12 @@ export function useReducer<S, A>(
   return [workInProgressHook.memoizedState, dispatch];
 }
 
-function pushEffect(tag, create, destroy, inputs) {
+function pushEffect(tag, create, destroy, deps) {
   const effect: Effect = {
     tag,
     create,
     destroy,
-    inputs,
+    deps,
     // Circular
     next: (null: any),
   };
@@ -516,34 +516,36 @@ export function useRef<T>(initialValue: T): {current: T} {
 
 export function useLayoutEffect(
   create: () => mixed,
-  inputs: Array<mixed> | void | null,
+  deps: Array<mixed> | void | null,
 ): void {
-  useEffectImpl(UpdateEffect, UnmountMutation | MountLayout, create, inputs);
+  useEffectImpl(UpdateEffect, UnmountMutation | MountLayout, create, deps);
 }
 
 export function useEffect(
   create: () => mixed,
-  inputs: Array<mixed> | void | null,
+  deps: Array<mixed> | void | null,
 ): void {
   useEffectImpl(
     UpdateEffect | PassiveEffect,
     UnmountPassive | MountPassive,
     create,
-    inputs,
+    deps,
   );
 }
 
-function useEffectImpl(fiberEffectTag, hookEffectTag, create, inputs): void {
+function useEffectImpl(fiberEffectTag, hookEffectTag, create, deps): void {
   currentlyRenderingFiber = resolveCurrentlyRenderingFiber();
   workInProgressHook = createWorkInProgressHook();
 
-  let nextInputs = inputs !== undefined && inputs !== null ? inputs : [create];
+  const nextDeps = deps === undefined ? null : deps;
   let destroy = null;
   if (currentHook !== null) {
     const prevEffect = currentHook.memoizedState;
     destroy = prevEffect.destroy;
-    if (areHookInputsEqual(nextInputs, prevEffect.inputs)) {
-      pushEffect(NoHookEffect, create, destroy, nextInputs);
+    // Assume these are defined. If they're not, areHookInputsEqual will warn.
+    const prevDeps: Array<mixed> = (prevEffect.deps: any);
+    if (nextDeps !== null && areHookInputsEqual(nextDeps, prevDeps)) {
+      pushEffect(NoHookEffect, create, destroy, nextDeps);
       return;
     }
   }
@@ -553,20 +555,18 @@ function useEffectImpl(fiberEffectTag, hookEffectTag, create, inputs): void {
     hookEffectTag,
     create,
     destroy,
-    nextInputs,
+    nextDeps,
   );
 }
 
 export function useImperativeHandle<T>(
   ref: {current: T | null} | ((inst: T | null) => mixed) | null | void,
   create: () => T,
-  inputs: Array<mixed> | void | null,
+  deps: Array<mixed> | void | null,
 ): void {
-  // TODO: If inputs are provided, should we skip comparing the ref itself?
-  const nextInputs =
-    inputs !== null && inputs !== undefined
-      ? inputs.concat([ref])
-      : [ref, create];
+  // TODO: If deps are provided, should we skip comparing the ref itself?
+  const nextDeps =
+    deps !== null && deps !== undefined ? deps.concat([ref]) : [ref];
 
   // TODO: I've implemented this on top of useEffect because it's almost the
   // same thing, and it would require an equal amount of code. It doesn't seem
@@ -585,7 +585,7 @@ export function useImperativeHandle<T>(
         refObject.current = null;
       };
     }
-  }, nextInputs);
+  }, nextDeps);
 }
 
 export function useDebugValue(
@@ -599,45 +599,45 @@ export function useDebugValue(
 
 export function useCallback<T>(
   callback: T,
-  inputs: Array<mixed> | void | null,
+  deps: Array<mixed> | void | null,
 ): T {
   currentlyRenderingFiber = resolveCurrentlyRenderingFiber();
   workInProgressHook = createWorkInProgressHook();
 
-  const nextInputs =
-    inputs !== undefined && inputs !== null ? inputs : [callback];
+  const nextDeps = deps === undefined ? null : deps;
 
   const prevState = workInProgressHook.memoizedState;
   if (prevState !== null) {
-    const prevInputs = prevState[1];
-    if (areHookInputsEqual(nextInputs, prevInputs)) {
+    // Assume these are defined. If they're not, areHookInputsEqual will warn.
+    const prevDeps: Array<mixed> = prevState[1];
+    if (nextDeps !== null && areHookInputsEqual(nextDeps, prevDeps)) {
       return prevState[0];
     }
   }
-  workInProgressHook.memoizedState = [callback, nextInputs];
+  workInProgressHook.memoizedState = [callback, nextDeps];
   return callback;
 }
 
 export function useMemo<T>(
   nextCreate: () => T,
-  inputs: Array<mixed> | void | null,
+  deps: Array<mixed> | void | null,
 ): T {
   currentlyRenderingFiber = resolveCurrentlyRenderingFiber();
   workInProgressHook = createWorkInProgressHook();
 
-  const nextInputs =
-    inputs !== undefined && inputs !== null ? inputs : [nextCreate];
+  const nextDeps = deps === undefined ? null : deps;
 
   const prevState = workInProgressHook.memoizedState;
   if (prevState !== null) {
-    const prevInputs = prevState[1];
-    if (areHookInputsEqual(nextInputs, prevInputs)) {
+    // Assume these are defined. If they're not, areHookInputsEqual will warn.
+    const prevDeps = prevState[1];
+    if (nextDeps !== null && areHookInputsEqual(nextDeps, prevDeps)) {
       return prevState[0];
     }
   }
 
   const nextValue = nextCreate();
-  workInProgressHook.memoizedState = [nextValue, nextInputs];
+  workInProgressHook.memoizedState = [nextValue, nextDeps];
   return nextValue;
 }
 

packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js

@@ -48,12 +48,42 @@ describe('ReactHooks', () => {
     }).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',
+        'Previous: [A, B]\n' +
+        'Incoming: [A]',
     ]);
     expect(ReactTestRenderer).toHaveYielded(['Did commit: A, B']);
   });
 
+  it('warns if switching from dependencies to no dependencies', () => {
+    spyOnDev(console, 'error');
+
+    const {useMemo} = React;
+    function App({text, hasDeps}) {
+      const resolvedText = useMemo(() => {
+        ReactTestRenderer.unstable_yield('Compute');
+        return text.toUpperCase();
+      }, hasDeps ? null : [text]);
+      return resolvedText;
+    }
+
+    const root = ReactTestRenderer.create(null);
+    root.update(<App text="Hello" hasDeps={true} />);
+    expect(ReactTestRenderer).toHaveYielded(['Compute']);
+    expect(root).toMatchRenderedOutput('HELLO');
+
+    expect(() => {
+      // This prints a warning message then throws a null access error
+      root.update(<App text="Hello" hasDeps={false} />);
+    }).toThrow('null');
+
+    if (__DEV__) {
+      expect(console.error).toHaveBeenCalledTimes(3);
+      expect(console.error.calls.argsFor(0)[0]).toContain(
+        'Warning: Detected a variable number of hook dependencies.',
+      );
+    }
+  });
+
   it('warns for bad useEffect return values', () => {
     const {useLayoutEffect} = React;
     function App(props) {

packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.internal.js

@@ -1015,11 +1015,11 @@ describe('ReactHooksWithNoopRenderer', () => {
       expect(ReactNoop.getChildren()).toEqual([]);
     });
 
-    it('skips effect if constructor has not changed', () => {
+    it('always fires effects if no dependencies are provided', () => {
       function effect() {
-        ReactNoop.yield(`Did mount`);
+        ReactNoop.yield(`Did create`);
         return () => {
-          ReactNoop.yield(`Did unmount`);
+          ReactNoop.yield(`Did destroy`);
         };
       }
       function Counter(props) {
@@ -1030,15 +1030,16 @@ describe('ReactHooksWithNoopRenderer', () => {
       expect(ReactNoop.flush()).toEqual(['Count: 0']);
       expect(ReactNoop.getChildren()).toEqual([span('Count: 0')]);
       flushPassiveEffects();
-      expect(ReactNoop.clearYields()).toEqual(['Did mount']);
+      expect(ReactNoop.clearYields()).toEqual(['Did create']);
 
       ReactNoop.render(<Counter count={1} />);
-      // No effect, because constructor was hoisted outside render
       expect(ReactNoop.flush()).toEqual(['Count: 1']);
       expect(ReactNoop.getChildren()).toEqual([span('Count: 1')]);
+      flushPassiveEffects();
+      expect(ReactNoop.clearYields()).toEqual(['Did destroy', 'Did create']);
 
       ReactNoop.render(null);
-      expect(ReactNoop.flush()).toEqual(['Did unmount']);
+      expect(ReactNoop.flush()).toEqual(['Did destroy']);
       expect(ReactNoop.getChildren()).toEqual([]);
     });
 
@@ -1456,7 +1457,7 @@ describe('ReactHooksWithNoopRenderer', () => {
       expect(ReactNoop.getChildren()).toEqual([span('GOODBYE')]);
     });
 
-    it('compares function if no inputs are provided', () => {
+    it('always re-computes if no inputs are provided', () => {
       function LazyCompute(props) {
         const computed = useMemo(props.compute);
         return <Text text={computed} />;
@@ -1476,10 +1477,10 @@ describe('ReactHooksWithNoopRenderer', () => {
       expect(ReactNoop.flush()).toEqual(['compute A', 'A']);
 
       ReactNoop.render(<LazyCompute compute={computeA} />);
-      expect(ReactNoop.flush()).toEqual(['A']);
+      expect(ReactNoop.flush()).toEqual(['compute A', 'A']);
 
       ReactNoop.render(<LazyCompute compute={computeA} />);
-      expect(ReactNoop.flush()).toEqual(['A']);
+      expect(ReactNoop.flush()).toEqual(['compute A', 'A']);
 
       ReactNoop.render(<LazyCompute compute={computeB} />);
       expect(ReactNoop.flush()).toEqual(['compute B', 'B']);

packages/shared/areHookInputsEqual.js

@@ -14,15 +14,17 @@ export default function areHookInputsEqual(arr1: any[], arr2: any[]) {
   // 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(', '),
-    );
+    if (arr1 === null || arr2 === null || arr1.length !== arr2.length) {
+      warning(
+        false,
+        '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 === null ? '(empty)' : `[${arr1.join(', ')}]`,
+        arr2 === null ? '(empty)' : `[${arr2.join(', ')}]`,
+      );
+    }
   }
   for (let i = 0; i < arr1.length; i++) {
     if (is(arr1[i], arr2[i])) {