From b1d648c259fd4681df3fdc69c998454e8aba8824 Mon Sep 17 00:00:00 2001 From: Brian Vaughn Date: Wed, 8 Apr 2020 14:33:59 -0700 Subject: [PATCH] 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. --- ...DOMServerIntegrationHooks-test.internal.js | 3 + .../react-reconciler/src/ReactFiberHooks.js | 34 ++++- .../src/__tests__/useRef-test.internal.js | 127 ++++++++++++++++++ 3 files changed, 161 insertions(+), 3 deletions(-) diff --git a/packages/react-dom/src/__tests__/ReactDOMServerIntegrationHooks-test.internal.js b/packages/react-dom/src/__tests__/ReactDOMServerIntegrationHooks-test.internal.js index 06fca95894aeb..e6f6d5f02229c 100644 --- 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 Count: {ref.current}; } + // Reading from ref during render (after a mutation) triggers a warning. + spyOnDev(console, 'warn'); + const domNode = await render(); expect(clearYields()).toEqual([0, 1, 2, 3]); expect(domNode.textContent).toEqual('Count: 3'); diff --git a/packages/react-reconciler/src/ReactFiberHooks.js b/packages/react-reconciler/src/ReactFiberHooks.js index d23df4be95158..030144d4eae5f 100644 --- a/packages/react-reconciler/src/ReactFiberHooks.js +++ b/packages/react-reconciler/src/ReactFiberHooks.js @@ -37,6 +37,7 @@ import {NoWork, Sync} from './ReactFiberExpirationTime'; import {NoMode, BlockingMode} from './ReactTypeOfMode'; import {readContext} from './ReactFiberNewContext'; import {createDeprecatedResponderListener} from './ReactFiberDeprecatedEvents'; +import {getStackByFiberInDevAndProd} from './ReactFiberComponentStack'; import { Update as UpdateEffect, Passive as PassiveEffect, @@ -1233,12 +1234,39 @@ function pushEffect(tag, create, destroy, deps) { function mountRef(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(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 index 1e2a48bd91069..ef8f5aadef55a 100644 --- 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,129 @@ describe('useRef', () => { ReactNoop.render(); 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(); + }); + }); + + 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(); + }); + }); + + it('should not warn about lazy init during render', () => { + function Example() { + const ref = useRef(null); + if (ref.current === null) { + // Read 1: safe because null + ref.current = 123; + } + return ref.current; // Read 2: safe because lazy init + } + + act(() => { + ReactNoop.render(); + }); + }); + + 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 ref = useRef(null); + useLayoutEffect(() => { + ref.current = 123; + setDidMount(true); + }, []); + return ref.current; // Read 2: safe because lazy init + } + + act(() => { + ReactNoop.render(); + }); + }); + + it('should warn about updates to ref after lazy init pattern', () => { + function Example() { + const ref = useRef(null); + if (ref.current === null) { + // Read 1: safe because null + ref.current = 123; + } + expect(ref.current).toBe(123); // Read 2: safe because lazy init + + ref.current = 456; // Second mutation, now reads will be unsafe + + expect(() => { + expect(ref.current).toBe(456); // Read 3: unsafe because mutation + }).toWarnDev([ + 'Example: Unsafe read of a mutable value during render.', + ]); + + return null; + } + + act(() => { + ReactNoop.render(); + }); + }); + + it('should not warn about reads witin 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(); + }); + + 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(); + }); + + expect(ref.current).toBe(456); + }); + } });