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];
 }