useRef: Warn about reading or writing mutable values during render (#…

…18545) Reading or writing a ref value during render is only safe if you are implementing the lazy initialization pattern. Other types of reading are unsafe as the ref is a mutable source. Other types of writing are unsafe as they are effectively side effects. This change also refactors useTransition to no longer use a ref hook, but instead manage its own (stable) hook state.
facebook · Oct 19, 2020 · c59c3df · c59c3df
1 parent 75726fa
commit c59c3df
Show file tree

Hide file tree

Showing 15 changed files with 567 additions and 121 deletions.
diff --git a/packages/react-dom/src/__tests__/ReactDOMServerIntegrationHooks-test.js b/packages/react-dom/src/__tests__/ReactDOMServerIntegrationHooks-test.js
@@ -474,31 +474,29 @@ describe('ReactDOMServerHooks', () => {
   describe('useRef', () => {
     itRenders('basic render', async render => {
       function Counter(props) {
-        const count = useRef(0);
-        return <span>Count: {count.current}</span>;
+        const ref = useRef();
+        return <span ref={ref}>Hi</span>;
       }
 
       const domNode = await render(<Counter />);
-      expect(domNode.textContent).toEqual('Count: 0');
+      expect(domNode.textContent).toEqual('Hi');
     });
 
     itRenders(
       'multiple times when updates happen during the render phase',
       async render => {
         function Counter(props) {
           const [count, setCount] = useState(0);
-          const ref = useRef(count);
+          const ref = useRef();
 
           if (count < 3) {
             const newCount = count + 1;
-
-            ref.current = newCount;
             setCount(newCount);
           }
 
           yieldValue(count);
 
-          return <span>Count: {ref.current}</span>;
+          return <span ref={ref}>Count: {count}</span>;
         }
 
         const domNode = await render(<Counter />);
@@ -513,7 +511,7 @@ describe('ReactDOMServerHooks', () => {
         let firstRef = null;
         function Counter(props) {
           const [count, setCount] = useState(0);
-          const ref = useRef(count);
+          const ref = useRef();
           if (firstRef === null) {
             firstRef = ref;
           } else if (firstRef !== ref) {
@@ -528,12 +526,12 @@ describe('ReactDOMServerHooks', () => {
 
           yieldValue(count);
 
-          return <span>Count: {ref.current}</span>;
+          return <span ref={ref}>Count: {count}</span>;
         }
 
         const domNode = await render(<Counter />);
         expect(clearYields()).toEqual([0, 1, 2, 3]);
-        expect(domNode.textContent).toEqual('Count: 0');
+        expect(domNode.textContent).toEqual('Count: 3');
       },
     );
   });

diff --git a/packages/react-reconciler/src/ReactFiberHooks.new.js b/packages/react-reconciler/src/ReactFiberHooks.new.js
@@ -27,6 +27,7 @@ import {
   enableNewReconciler,
   decoupleUpdatePriorityFromScheduler,
   enableDoubleInvokingEffects,
+  enableUseRefAccessWarning,
 } from 'shared/ReactFeatureFlags';
 
 import {
@@ -1197,14 +1198,92 @@ function pushEffect(tag, create, destroy, deps) {
   return effect;
 }
 
+let stackContainsErrorMessage: boolean | null = null;
+
+function getCallerStackFrame(): string {
+  const stackFrames = new Error('Error message').stack.split('\n');
+
+  // Some browsers (e.g. Chrome) include the error message in the stack
+  // but others (e.g. Firefox) do not.
+  if (stackContainsErrorMessage === null) {
+    stackContainsErrorMessage = stackFrames[0].includes('Error message');
+  }
+
+  return stackContainsErrorMessage
+    ? stackFrames.slice(3, 4).join('\n')
+    : stackFrames.slice(2, 3).join('\n');
+}
+
 function mountRef<T>(initialValue: T): {|current: T|} {
   const hook = mountWorkInProgressHook();
-  const ref = {current: initialValue};
-  if (__DEV__) {
-    Object.seal(ref);
+  if (enableUseRefAccessWarning) {
+    if (__DEV__) {
+      // Support lazy initialization pattern shown in docs.
+      // We need to store the caller stack frame so that we don't warn on subsequent renders.
+      let hasBeenInitialized = initialValue != null;
+      let lazyInitGetterStack = null;
+      let didCheckForLazyInit = false;
+
+      // Only warn once per component+hook.
+      let didWarnAboutRead = false;
+      let didWarnAboutWrite = false;
+
+      let current = initialValue;
+      const ref = {
+        get current() {
+          if (!hasBeenInitialized) {
+            didCheckForLazyInit = true;
+            lazyInitGetterStack = getCallerStackFrame();
+          } else if (currentlyRenderingFiber !== null && !didWarnAboutRead) {
+            if (
+              lazyInitGetterStack === null ||
+              lazyInitGetterStack !== getCallerStackFrame()
+            ) {
+              didWarnAboutRead = true;
+              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',
+                getComponentName(currentlyRenderingFiber.type) || 'Unknown',
+              );
+            }
+          }
+          return current;
+        },
+        set current(value) {
+          if (currentlyRenderingFiber !== null && !didWarnAboutWrite) {
+            if (
+              hasBeenInitialized ||
+              (!hasBeenInitialized && !didCheckForLazyInit)
+            ) {
+              didWarnAboutWrite = true;
+              console.warn(
+                '%s: Unsafe write of a mutable value during render.\n\n' +
+                  'Writing to a ref during render is only safe if the ref holds ' +
+                  'a lazily-initialized value that is only set once.\n',
+                getComponentName(currentlyRenderingFiber.type) || 'Unknown',
+              );
+            }
+          }
+
+          hasBeenInitialized = true;
+          current = value;
+        },
+      };
+      Object.seal(ref);
+      hook.memoizedState = ref;
+      return ref;
+    } else {
+      const ref = {current: initialValue};
+      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|} {
@@ -1591,24 +1670,24 @@ function startTransition(setPending, callback) {
 
 function mountTransition(): [(() => void) => void, boolean] {
   const [isPending, setPending] = mountState(false);
-  // The `start` method can be stored on a ref, since `setPending`
-  // never changes.
+  // The `start` method never changes.
   const start = startTransition.bind(null, setPending);
-  mountRef(start);
+  const hook = mountWorkInProgressHook();
+  hook.memoizedState = start;
   return [start, isPending];
 }
 
 function updateTransition(): [(() => void) => void, boolean] {
   const [isPending] = updateState(false);
-  const startRef = updateRef();
-  const start: (() => void) => void = (startRef.current: any);
+  const hook = updateWorkInProgressHook();
+  const start = hook.memoizedState;
   return [start, isPending];
 }
 
 function rerenderTransition(): [(() => void) => void, boolean] {
   const [isPending] = rerenderState(false);
-  const startRef = updateRef();
-  const start: (() => void) => void = (startRef.current: any);
+  const hook = updateWorkInProgressHook();
+  const start = hook.memoizedState;
   return [start, isPending];
 }
 

diff --git a/packages/react-reconciler/src/ReactFiberHooks.old.js b/packages/react-reconciler/src/ReactFiberHooks.old.js
@@ -26,6 +26,7 @@ import {
   enableSchedulingProfiler,
   enableNewReconciler,
   decoupleUpdatePriorityFromScheduler,
+  enableUseRefAccessWarning,
 } from 'shared/ReactFeatureFlags';
 
 import {NoMode, BlockingMode, DebugTracingMode} from './ReactTypeOfMode';
@@ -1175,14 +1176,92 @@ function pushEffect(tag, create, destroy, deps) {
   return effect;
 }
 
+let stackContainsErrorMessage: boolean | null = null;
+
+function getCallerStackFrame(): string {
+  const stackFrames = new Error('Error message').stack.split('\n');
+
+  // Some browsers (e.g. Chrome) include the error message in the stack
+  // but others (e.g. Firefox) do not.
+  if (stackContainsErrorMessage === null) {
+    stackContainsErrorMessage = stackFrames[0].includes('Error message');
+  }
+
+  return stackContainsErrorMessage
+    ? stackFrames.slice(3, 4).join('\n')
+    : stackFrames.slice(2, 3).join('\n');
+}
+
 function mountRef<T>(initialValue: T): {|current: T|} {
   const hook = mountWorkInProgressHook();
-  const ref = {current: initialValue};
-  if (__DEV__) {
-    Object.seal(ref);
+  if (enableUseRefAccessWarning) {
+    if (__DEV__) {
+      // Support lazy initialization pattern shown in docs.
+      // We need to store the caller stack frame so that we don't warn on subsequent renders.
+      let hasBeenInitialized = initialValue != null;
+      let lazyInitGetterStack = null;
+      let didCheckForLazyInit = false;
+
+      // Only warn once per component+hook.
+      let didWarnAboutRead = false;
+      let didWarnAboutWrite = false;
+
+      let current = initialValue;
+      const ref = {
+        get current() {
+          if (!hasBeenInitialized) {
+            didCheckForLazyInit = true;
+            lazyInitGetterStack = getCallerStackFrame();
+          } else if (currentlyRenderingFiber !== null && !didWarnAboutRead) {
+            if (
+              lazyInitGetterStack === null ||
+              lazyInitGetterStack !== getCallerStackFrame()
+            ) {
+              didWarnAboutRead = true;
+              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',
+                getComponentName(currentlyRenderingFiber.type) || 'Unknown',
+              );
+            }
+          }
+          return current;
+        },
+        set current(value) {
+          if (currentlyRenderingFiber !== null && !didWarnAboutWrite) {
+            if (
+              hasBeenInitialized ||
+              (!hasBeenInitialized && !didCheckForLazyInit)
+            ) {
+              didWarnAboutWrite = true;
+              console.warn(
+                '%s: Unsafe write of a mutable value during render.\n\n' +
+                  'Writing to a ref during render is only safe if the ref holds ' +
+                  'a lazily-initialized value that is only set once.\n',
+                getComponentName(currentlyRenderingFiber.type) || 'Unknown',
+              );
+            }
+          }
+
+          hasBeenInitialized = true;
+          current = value;
+        },
+      };
+      Object.seal(ref);
+      hook.memoizedState = ref;
+      return ref;
+    } else {
+      const ref = {current: initialValue};
+      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|} {
@@ -1534,24 +1613,24 @@ function startTransition(setPending, callback) {
 
 function mountTransition(): [(() => void) => void, boolean] {
   const [isPending, setPending] = mountState(false);
-  // The `start` method can be stored on a ref, since `setPending`
-  // never changes.
+  // The `start` method never changes.
   const start = startTransition.bind(null, setPending);
-  mountRef(start);
+  const hook = mountWorkInProgressHook();
+  hook.memoizedState = start;
   return [start, isPending];
 }
 
 function updateTransition(): [(() => void) => void, boolean] {
   const [isPending] = updateState(false);
-  const startRef = updateRef();
-  const start: (() => void) => void = (startRef.current: any);
+  const hook = updateWorkInProgressHook();
+  const start = hook.memoizedState;
   return [start, isPending];
 }
 
 function rerenderTransition(): [(() => void) => void, boolean] {
   const [isPending] = rerenderState(false);
-  const startRef = updateRef();
-  const start: (() => void) => void = (startRef.current: any);
+  const hook = updateWorkInProgressHook();
+  const start = hook.memoizedState;
   return [start, isPending];
 }