useRef: Warn if reading mutable value during render

Reading from a ref during render is only safe if: 1. The ref value has not been updated, or 2. The ref holds a lazily-initialized value that is only set once. This PR adds a new DEV warning to detect unsaf reads.
facebook · Apr 9, 2020 · b3dc63b · b3dc63b
1 parent 990da53
commit b3dc63b
Show file tree

Hide file tree

Showing 4 changed files with 205 additions and 6 deletions.
diff --git a/packages/react-dom/src/__tests__/ReactDOMServerIntegrationHooks-test.internal.js b/packages/react-dom/src/__tests__/ReactDOMServerIntegrationHooks-test.internal.js
@@ -503,6 +503,9 @@ describe('ReactDOMServerHooks', () => {
           return <span>Count: {ref.current}</span>;
         }
 
+        // Reading from ref during render (after a mutation) triggers a warning.
+        spyOnDev(console, 'warn');
+
         const domNode = await render(<Counter />);
         expect(clearYields()).toEqual([0, 1, 2, 3]);
         expect(domNode.textContent).toEqual('Count: 3');

diff --git a/packages/react-reconciler/src/ReactFiberHooks.new.js b/packages/react-reconciler/src/ReactFiberHooks.new.js
@@ -37,6 +37,7 @@ import {NoWork, Sync} from './ReactFiberExpirationTime';
 import {NoMode, BlockingMode} from './ReactTypeOfMode';
 import {readContext} from './ReactFiberNewContext.new';
 import {createDeprecatedResponderListener} from './ReactFiberDeprecatedEvents.new';
+import {getStackByFiberInDevAndProd} from './ReactFiberComponentStack';
 import {
   Update as UpdateEffect,
   Passive as PassiveEffect,
@@ -1184,12 +1185,39 @@ function pushEffect(tag, create, destroy, deps) {
 
 function mountRef<T>(initialValue: T): {|current: T|} {
   const hook = mountWorkInProgressHook();
-  const ref = {current: initialValue};
   if (__DEV__) {
+    const fiber = currentlyRenderingFiber;
+    let current = initialValue;
+    let shouldWarnAboutRead = false;
+    const ref = {
+      get current() {
+        if (shouldWarnAboutRead && currentlyRenderingFiber !== null) {
+          console.warn(
+            '%s: Unsafe read of a mutable value during render.\n\n' +
+              'Reading from a ref during render is only safe if:\n' +
+              '1. The ref value has not been updated, or\n' +
+              '2. The ref holds a lazily-initialized value that is only set once.\n\n%s',
+            getComponentName(fiber.type) || 'Unknown',
+            getStackByFiberInDevAndProd(fiber),
+          );
+        }
+        return current;
+      },
+      set current(value) {
+        if (!shouldWarnAboutRead && current != null) {
+          shouldWarnAboutRead = true;
+        }
+        current = value;
+      },
+    };
     Object.seal(ref);
+    hook.memoizedState = ref;
+    return ref;
+  } else {
+    const ref = {current: initialValue};
+    hook.memoizedState = ref;
+    return ref;
   }
-  hook.memoizedState = ref;
-  return ref;
 }
 
 function updateRef<T>(initialValue: T): {|current: T|} {

diff --git a/packages/react-reconciler/src/ReactFiberHooks.old.js b/packages/react-reconciler/src/ReactFiberHooks.old.js
@@ -37,6 +37,7 @@ import {NoWork, Sync} from './ReactFiberExpirationTime';
 import {NoMode, BlockingMode} from './ReactTypeOfMode';
 import {readContext} from './ReactFiberNewContext.old';
 import {createDeprecatedResponderListener} from './ReactFiberDeprecatedEvents.old';
+import {getStackByFiberInDevAndProd} from './ReactFiberComponentStack';
 import {
   Update as UpdateEffect,
   Passive as PassiveEffect,
@@ -1184,12 +1185,39 @@ function pushEffect(tag, create, destroy, deps) {
 
 function mountRef<T>(initialValue: T): {|current: T|} {
   const hook = mountWorkInProgressHook();
-  const ref = {current: initialValue};
   if (__DEV__) {
+    const fiber = currentlyRenderingFiber;
+    let current = initialValue;
+    let shouldWarnAboutRead = false;
+    const ref = {
+      get current() {
+        if (shouldWarnAboutRead && currentlyRenderingFiber !== null) {
+          console.warn(
+            '%s: Unsafe read of a mutable value during render.\n\n' +
+              'Reading from a ref during render is only safe if:\n' +
+              '1. The ref value has not been updated, or\n' +
+              '2. The ref holds a lazily-initialized value that is only set once.\n\n%s',
+            getComponentName(fiber.type) || 'Unknown',
+            getStackByFiberInDevAndProd(fiber),
+          );
+        }
+        return current;
+      },
+      set current(value) {
+        if (!shouldWarnAboutRead && current != null) {
+          shouldWarnAboutRead = true;
+        }
+        current = value;
+      },
+    };
     Object.seal(ref);
+    hook.memoizedState = ref;
+    return ref;
+  } else {
+    const ref = {current: initialValue};
+    hook.memoizedState = ref;
+    return ref;
   }
-  hook.memoizedState = ref;
-  return ref;
 }
 
 function updateRef<T>(initialValue: T): {|current: T|} {

diff --git a/packages/react-reconciler/src/__tests__/useRef-test.internal.js b/packages/react-reconciler/src/__tests__/useRef-test.internal.js
@@ -19,6 +19,7 @@ describe('useRef', () => {
   let act;
   let useCallback;
   let useEffect;
+  let useLayoutEffect;
   let useRef;
   let useState;
 
@@ -33,6 +34,7 @@ describe('useRef', () => {
     act = ReactNoop.act;
     useCallback = React.useCallback;
     useEffect = React.useEffect;
+    useLayoutEffect = React.useLayoutEffect;
     useRef = React.useRef;
     useState = React.useState;
   });
@@ -124,4 +126,142 @@ describe('useRef', () => {
     ReactNoop.render(<Counter />);
     expect(Scheduler).toFlushAndYield(['val']);
   });
+
+  if (__DEV__) {
+    it('should not warn about reads if value is not mutated', () => {
+      function Example() {
+        const ref = useRef(123);
+        return ref.current;
+      }
+
+      act(() => {
+        ReactNoop.render(<Example />);
+      });
+    });
+
+    it('should warn about reads during render phase if value has been mutated', () => {
+      function Example() {
+        const ref = useRef(123);
+        ref.current = 456;
+
+        let value;
+        expect(() => {
+          value = ref.current;
+        }).toWarnDev([
+          'Example: Unsafe read of a mutable value during render.',
+        ]);
+
+        return value;
+      }
+
+      act(() => {
+        ReactNoop.render(<Example />);
+      });
+    });
+
+    it('should not warn about lazy init during render', () => {
+      function Example() {
+        const ref1 = useRef(null);
+        const ref2 = useRef();
+        if (ref1.current === null) {
+          // Read 1: safe because null
+          ref1.current = 123;
+          ref2.current = 123;
+        }
+        return ref1.current + ref2.current; // Read 2: safe because lazy init
+      }
+
+      act(() => {
+        ReactNoop.render(<Example />);
+      });
+    });
+
+    it('should not warn about lazy init outside of render', () => {
+      function Example() {
+        // eslint-disable-next-line no-unused-vars
+        const [didMount, setDidMount] = useState(false);
+        const ref1 = useRef(null);
+        const ref2 = useRef();
+        useLayoutEffect(() => {
+          ref1.current = 123;
+          ref2.current = 123;
+          setDidMount(true);
+        }, []);
+        return ref1.current + ref2.current; // Read 2: safe because lazy init
+      }
+
+      act(() => {
+        ReactNoop.render(<Example />);
+      });
+    });
+
+    it('should warn about updates to ref after lazy init pattern', () => {
+      function Example() {
+        const ref1 = useRef(null);
+        const ref2 = useRef();
+        if (ref1.current === null) {
+          // Read 1: safe because null
+          ref1.current = 123;
+          ref2.current = 123;
+        }
+        expect(ref1.current).toBe(123); // Read 2: safe because lazy init
+        expect(ref2.current).toBe(123); // Read 2: safe because lazy init
+
+        ref1.current = 456; // Second mutation, now reads will be unsafe
+        ref2.current = 456; // Second mutation, now reads will be unsafe
+
+        expect(() => {
+          expect(ref1.current).toBe(456); // Read 3: unsafe because mutation
+        }).toWarnDev([
+          'Example: Unsafe read of a mutable value during render.',
+        ]);
+        expect(() => {
+          expect(ref2.current).toBe(456); // Read 3: unsafe because mutation
+        }).toWarnDev([
+          'Example: Unsafe read of a mutable value during render.',
+        ]);
+
+        return null;
+      }
+
+      act(() => {
+        ReactNoop.render(<Example />);
+      });
+    });
+
+    it('should not warn about reads within effect', () => {
+      function Example() {
+        const ref = useRef(123);
+        ref.current = 456;
+        useLayoutEffect(() => {
+          expect(ref.current).toBe(456);
+        }, []);
+        useEffect(() => {
+          expect(ref.current).toBe(456);
+        }, []);
+        return null;
+      }
+
+      act(() => {
+        ReactNoop.render(<Example />);
+      });
+
+      ReactNoop.flushPassiveEffects();
+    });
+
+    it('should not warn about reads outside of render phase (e.g. event handler)', () => {
+      let ref;
+      function Example() {
+        ref = useRef(123);
+        ref.current = 456;
+        return null;
+      }
+
+      act(() => {
+        ReactNoop.render(<Example />);
+      });
+
+      expect(ref.current).toBe(456);
+    });
+  }
 });