From 32e5c1b957593a126a16844e45c0deb926bb9af1 Mon Sep 17 00:00:00 2001 From: Brian Vaughn Date: Sun, 18 Oct 2020 09:20:16 -0400 Subject: [PATCH 1/2] useRef: Warn about reading or writing mutable values during render 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. --- .../ReactDOMServerIntegrationHooks-test.js | 18 +- .../src/ReactFiberHooks.new.js | 103 ++++- .../src/ReactFiberHooks.old.js | 103 ++++- .../ReactHooksWithNoopRenderer-test.js | 89 +---- .../src/__tests__/useRef-test.internal.js | 361 ++++++++++++++++++ packages/shared/ReactFeatureFlags.js | 2 + .../forks/ReactFeatureFlags.native-fb.js | 1 + .../forks/ReactFeatureFlags.native-oss.js | 1 + .../forks/ReactFeatureFlags.test-renderer.js | 1 + .../ReactFeatureFlags.test-renderer.native.js | 1 + .../ReactFeatureFlags.test-renderer.www.js | 1 + .../shared/forks/ReactFeatureFlags.testing.js | 1 + .../forks/ReactFeatureFlags.testing.www.js | 1 + .../forks/ReactFeatureFlags.www-dynamic.js | 1 + .../shared/forks/ReactFeatureFlags.www.js | 1 + 15 files changed, 564 insertions(+), 121 deletions(-) create mode 100644 packages/react-reconciler/src/__tests__/useRef-test.internal.js diff --git a/packages/react-dom/src/__tests__/ReactDOMServerIntegrationHooks-test.js b/packages/react-dom/src/__tests__/ReactDOMServerIntegrationHooks-test.js index 6cb57a8560b..397b739546d 100644 --- a/packages/react-dom/src/__tests__/ReactDOMServerIntegrationHooks-test.js +++ b/packages/react-dom/src/__tests__/ReactDOMServerIntegrationHooks-test.js @@ -474,12 +474,12 @@ describe('ReactDOMServerHooks', () => { describe('useRef', () => { itRenders('basic render', async render => { function Counter(props) { - const count = useRef(0); - return Count: {count.current}; + const ref = useRef(); + return Hi; } const domNode = await render(); - expect(domNode.textContent).toEqual('Count: 0'); + expect(domNode.textContent).toEqual('Hi'); }); itRenders( @@ -487,18 +487,16 @@ describe('ReactDOMServerHooks', () => { 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 Count: {ref.current}; + return Count: {count}; } const domNode = await render(); @@ -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 Count: {ref.current}; + return Count: {count}; } const domNode = await render(); 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 index bdd0bed4c75..9126ee1687a 100644 --- a/packages/react-reconciler/src/ReactFiberHooks.new.js +++ b/packages/react-reconciler/src/ReactFiberHooks.new.js @@ -27,6 +27,7 @@ import { enableNewReconciler, decoupleUpdatePriorityFromScheduler, enableDoubleInvokingEffects, + enableUseRefMutationWarning, } 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(initialValue: T): {|current: T|} { const hook = mountWorkInProgressHook(); - const ref = {current: initialValue}; - if (__DEV__) { - Object.seal(ref); + if (enableUseRefMutationWarning) { + 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(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 index 972a7464027..826ae84cf76 100644 --- a/packages/react-reconciler/src/ReactFiberHooks.old.js +++ b/packages/react-reconciler/src/ReactFiberHooks.old.js @@ -26,6 +26,7 @@ import { enableSchedulingProfiler, enableNewReconciler, decoupleUpdatePriorityFromScheduler, + enableUseRefMutationWarning, } 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(initialValue: T): {|current: T|} { const hook = mountWorkInProgressHook(); - const ref = {current: initialValue}; - if (__DEV__) { - Object.seal(ref); + if (enableUseRefMutationWarning) { + 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(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]; } diff --git a/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.js b/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.js index 333e8930e73..79794bbea7f 100644 --- a/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.js +++ b/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.js @@ -1536,7 +1536,7 @@ describe('ReactHooksWithNoopRenderer', () => { it('does not show a warning when a component updates a childs state from within passive unmount function', () => { function Parent() { Scheduler.unstable_yieldValue('Parent'); - const updaterRef = React.useRef(null); + const updaterRef = useRef(null); React.useEffect(() => { Scheduler.unstable_yieldValue('Parent passive create'); return () => { @@ -2612,7 +2612,7 @@ describe('ReactHooksWithNoopRenderer', () => { }); // @gate new - it('should skip unmounted boundaries and use the nearest still-mounted boundary', () => { + it('should skip unmounted boundaries and use the nearest still-mounted boundary', () => { function Conditional({showChildren}) { if (showChildren) { return ( @@ -3202,91 +3202,6 @@ describe('ReactHooksWithNoopRenderer', () => { }); }); - describe('useRef', () => { - it('creates a ref object initialized with the provided value', () => { - jest.useFakeTimers(); - - function useDebouncedCallback(callback, ms, inputs) { - const timeoutID = useRef(-1); - useEffect(() => { - return function unmount() { - clearTimeout(timeoutID.current); - }; - }, []); - const debouncedCallback = useCallback( - (...args) => { - clearTimeout(timeoutID.current); - timeoutID.current = setTimeout(callback, ms, ...args); - }, - [callback, ms], - ); - return useCallback(debouncedCallback, inputs); - } - - let ping; - function App() { - ping = useDebouncedCallback( - value => { - Scheduler.unstable_yieldValue('ping: ' + value); - }, - 100, - [], - ); - return null; - } - - act(() => { - ReactNoop.render(); - }); - expect(Scheduler).toHaveYielded([]); - - ping(1); - ping(2); - ping(3); - - expect(Scheduler).toHaveYielded([]); - - jest.advanceTimersByTime(100); - - expect(Scheduler).toHaveYielded(['ping: 3']); - - ping(4); - jest.advanceTimersByTime(20); - ping(5); - ping(6); - jest.advanceTimersByTime(80); - - expect(Scheduler).toHaveYielded([]); - - jest.advanceTimersByTime(20); - expect(Scheduler).toHaveYielded(['ping: 6']); - }); - - it('should return the same ref during re-renders', () => { - function Counter() { - const ref = useRef('val'); - const [count, setCount] = useState(0); - const [firstRef] = useState(ref); - - if (firstRef !== ref) { - throw new Error('should never change'); - } - - if (count < 3) { - setCount(count + 1); - } - - return ; - } - - ReactNoop.render(); - expect(Scheduler).toFlushAndYield(['val']); - - ReactNoop.render(); - expect(Scheduler).toFlushAndYield(['val']); - }); - }); - describe('useImperativeHandle', () => { it('does not update when deps are the same', () => { const INCREMENT = 'INCREMENT'; diff --git a/packages/react-reconciler/src/__tests__/useRef-test.internal.js b/packages/react-reconciler/src/__tests__/useRef-test.internal.js new file mode 100644 index 00000000000..b072887a128 --- /dev/null +++ b/packages/react-reconciler/src/__tests__/useRef-test.internal.js @@ -0,0 +1,361 @@ +/** + * Copyright (c) Facebook, Inc. and its affiliates. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + * + * @emails react-core + * @jest-environment node + */ + +/* eslint-disable no-func-assign */ + +'use strict'; + +describe('useRef', () => { + let React; + let ReactNoop; + let Scheduler; + let act; + let useCallback; + let useEffect; + let useLayoutEffect; + let useRef; + let useState; + + beforeEach(() => { + React = require('react'); + ReactNoop = require('react-noop-renderer'); + Scheduler = require('scheduler'); + + const ReactFeatureFlags = require('shared/ReactFeatureFlags'); + ReactFeatureFlags.debugRenderPhaseSideEffectsForStrictMode = false; + ReactFeatureFlags.enableUseRefMutationWarning = true; + + act = ReactNoop.act; + useCallback = React.useCallback; + useEffect = React.useEffect; + useLayoutEffect = React.useLayoutEffect; + useRef = React.useRef; + useState = React.useState; + }); + + function Text(props) { + Scheduler.unstable_yieldValue(props.text); + return ; + } + + it('creates a ref object initialized with the provided value', () => { + jest.useFakeTimers(); + + function useDebouncedCallback(callback, ms, inputs) { + const timeoutID = useRef(-1); + useEffect(() => { + return function unmount() { + clearTimeout(timeoutID.current); + }; + }, []); + const debouncedCallback = useCallback( + (...args) => { + clearTimeout(timeoutID.current); + timeoutID.current = setTimeout(callback, ms, ...args); + }, + [callback, ms], + ); + return useCallback(debouncedCallback, inputs); + } + + let ping; + function App() { + ping = useDebouncedCallback( + value => { + Scheduler.unstable_yieldValue('ping: ' + value); + }, + 100, + [], + ); + return null; + } + + act(() => { + ReactNoop.render(); + }); + expect(Scheduler).toHaveYielded([]); + + ping(1); + ping(2); + ping(3); + + expect(Scheduler).toHaveYielded([]); + + jest.advanceTimersByTime(100); + + expect(Scheduler).toHaveYielded(['ping: 3']); + + ping(4); + jest.advanceTimersByTime(20); + ping(5); + ping(6); + jest.advanceTimersByTime(80); + + expect(Scheduler).toHaveYielded([]); + + jest.advanceTimersByTime(20); + expect(Scheduler).toHaveYielded(['ping: 6']); + }); + + it('should never warn when attaching to children', () => { + class Component extends React.Component { + render() { + return null; + } + } + + function Example({phase}) { + const hostRef = useRef(); + const classRef = useRef(); + return ( + <> +
+ + + ); + } + + act(() => { + ReactNoop.render(); + }); + act(() => { + ReactNoop.render(); + }); + }); + + it('should return the same ref during re-renders', () => { + function Counter() { + const ref = useRef('val'); + const [count, setCount] = useState(0); + const [firstRef] = useState(ref); + + if (firstRef !== ref) { + throw new Error('should never change'); + } + + if (count < 3) { + setCount(count + 1); + } + + return ; + } + + ReactNoop.render(); + expect(Scheduler).toFlushAndYield([3]); + + ReactNoop.render(); + expect(Scheduler).toFlushAndYield([3]); + }); + + if (__DEV__) { + it('should warn about reads during render', () => { + function Example() { + const ref = useRef(123); + 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 ref1 = useRef(null); + const ref2 = useRef(undefined); + // Read: safe because lazy init: + if (ref1.current === null) { + ref1.current = 123; + } + if (ref2.current === undefined) { + ref2.current = 123; + } + return null; + } + + act(() => { + ReactNoop.render(); + }); + + // Should not warn after an update either. + 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 ref1 = useRef(null); + const ref2 = useRef(undefined); + useLayoutEffect(() => { + ref1.current = 123; + ref2.current = 123; + setDidMount(true); + }, []); + return null; + } + + act(() => { + ReactNoop.render(); + }); + }); + + it('should not warn about unconditional lazy init during render', () => { + function Example() { + const ref1 = useRef(null); + const ref2 = useRef(undefined); + + if (shouldExpectWarning) { + expect(() => { + ref1.current = 123; + }).toWarnDev([ + 'Example: Unsafe write of a mutable value during render', + ]); + expect(() => { + ref2.current = 123; + }).toWarnDev([ + 'Example: Unsafe write of a mutable value during render', + ]); + } else { + ref1.current = 123; + ref1.current = 123; + } + + // But only warn once + ref1.current = 345; + ref1.current = 345; + + return null; + } + + let shouldExpectWarning = true; + act(() => { + ReactNoop.render(); + }); + + // Should not warn again on update. + shouldExpectWarning = false; + act(() => { + ReactNoop.render(); + }); + }); + + it('should warn about reads to ref after lazy init pattern', () => { + function Example() { + const ref1 = useRef(null); + const ref2 = useRef(undefined); + + // Read 1: safe because lazy init: + if (ref1.current === null) { + ref1.current = 123; + } + if (ref2.current === undefined) { + ref2.current = 123; + } + + let value; + expect(() => { + value = ref1.current; + }).toWarnDev(['Example: Unsafe read of a mutable value during render']); + expect(() => { + value = ref2.current; + }).toWarnDev(['Example: Unsafe read of a mutable value during render']); + + // But it should only warn once. + value = ref1.current; + value = ref2.current; + + return value; + } + + act(() => { + ReactNoop.render(); + }); + }); + + it('should warn about writes to ref after lazy init pattern', () => { + function Example() { + const ref1 = useRef(null); + const ref2 = useRef(undefined); + // Read: safe because lazy init: + if (ref1.current === null) { + ref1.current = 123; + } + if (ref2.current === undefined) { + ref2.current = 123; + } + + expect(() => { + ref1.current = 456; + }).toWarnDev([ + 'Example: Unsafe write of a mutable value during render', + ]); + expect(() => { + ref2.current = 456; + }).toWarnDev([ + 'Example: Unsafe write of a mutable value during render', + ]); + + return null; + } + + act(() => { + ReactNoop.render(); + }); + }); + + it('should not warn about reads or writes within effect', () => { + function Example() { + const ref = useRef(123); + useLayoutEffect(() => { + expect(ref.current).toBe(123); + ref.current = 456; + expect(ref.current).toBe(456); + }, []); + useEffect(() => { + expect(ref.current).toBe(456); + ref.current = 789; + expect(ref.current).toBe(789); + }, []); + return null; + } + + act(() => { + ReactNoop.render(); + }); + + ReactNoop.flushPassiveEffects(); + }); + + it('should not warn about reads or writes outside of render phase (e.g. event handler)', () => { + let ref; + function Example() { + ref = useRef(123); + return null; + } + + act(() => { + ReactNoop.render(); + }); + + expect(ref.current).toBe(123); + ref.current = 456; + expect(ref.current).toBe(456); + }); + } +}); diff --git a/packages/shared/ReactFeatureFlags.js b/packages/shared/ReactFeatureFlags.js index 836b78524ef..68da7c2f3a0 100644 --- a/packages/shared/ReactFeatureFlags.js +++ b/packages/shared/ReactFeatureFlags.js @@ -134,3 +134,5 @@ export const decoupleUpdatePriorityFromScheduler = false; export const enableDiscreteEventFlushingChange = false; export const enableDoubleInvokingEffects = false; + +export const enableUseRefMutationWarning = false; diff --git a/packages/shared/forks/ReactFeatureFlags.native-fb.js b/packages/shared/forks/ReactFeatureFlags.native-fb.js index 82e8d4dcd7b..9ea390dc4cd 100644 --- a/packages/shared/forks/ReactFeatureFlags.native-fb.js +++ b/packages/shared/forks/ReactFeatureFlags.native-fb.js @@ -51,6 +51,7 @@ export const decoupleUpdatePriorityFromScheduler = false; export const enableDiscreteEventFlushingChange = false; export const enableDoubleInvokingEffects = false; +export const enableUseRefMutationWarning = false; // Flow magic to verify the exports of this file match the original version. // eslint-disable-next-line no-unused-vars diff --git a/packages/shared/forks/ReactFeatureFlags.native-oss.js b/packages/shared/forks/ReactFeatureFlags.native-oss.js index 7a793d16ab3..b9b6072db6f 100644 --- a/packages/shared/forks/ReactFeatureFlags.native-oss.js +++ b/packages/shared/forks/ReactFeatureFlags.native-oss.js @@ -50,6 +50,7 @@ export const decoupleUpdatePriorityFromScheduler = false; export const enableDiscreteEventFlushingChange = false; export const enableDoubleInvokingEffects = false; +export const enableUseRefMutationWarning = false; // Flow magic to verify the exports of this file match the original version. // eslint-disable-next-line no-unused-vars diff --git a/packages/shared/forks/ReactFeatureFlags.test-renderer.js b/packages/shared/forks/ReactFeatureFlags.test-renderer.js index 4402523c541..78470db3095 100644 --- a/packages/shared/forks/ReactFeatureFlags.test-renderer.js +++ b/packages/shared/forks/ReactFeatureFlags.test-renderer.js @@ -50,6 +50,7 @@ export const decoupleUpdatePriorityFromScheduler = false; export const enableDiscreteEventFlushingChange = false; export const enableDoubleInvokingEffects = false; +export const enableUseRefMutationWarning = false; // Flow magic to verify the exports of this file match the original version. // eslint-disable-next-line no-unused-vars diff --git a/packages/shared/forks/ReactFeatureFlags.test-renderer.native.js b/packages/shared/forks/ReactFeatureFlags.test-renderer.native.js index a71ad45dbd4..132b37de777 100644 --- a/packages/shared/forks/ReactFeatureFlags.test-renderer.native.js +++ b/packages/shared/forks/ReactFeatureFlags.test-renderer.native.js @@ -50,6 +50,7 @@ export const decoupleUpdatePriorityFromScheduler = false; export const enableDiscreteEventFlushingChange = false; export const enableDoubleInvokingEffects = false; +export const enableUseRefMutationWarning = false; // Flow magic to verify the exports of this file match the original version. // eslint-disable-next-line no-unused-vars diff --git a/packages/shared/forks/ReactFeatureFlags.test-renderer.www.js b/packages/shared/forks/ReactFeatureFlags.test-renderer.www.js index c60072346ea..9d5337dc8cb 100644 --- a/packages/shared/forks/ReactFeatureFlags.test-renderer.www.js +++ b/packages/shared/forks/ReactFeatureFlags.test-renderer.www.js @@ -50,6 +50,7 @@ export const decoupleUpdatePriorityFromScheduler = false; export const enableDiscreteEventFlushingChange = false; export const enableDoubleInvokingEffects = false; +export const enableUseRefMutationWarning = false; // Flow magic to verify the exports of this file match the original version. // eslint-disable-next-line no-unused-vars diff --git a/packages/shared/forks/ReactFeatureFlags.testing.js b/packages/shared/forks/ReactFeatureFlags.testing.js index 90c1fa268b2..1ea9f969a1a 100644 --- a/packages/shared/forks/ReactFeatureFlags.testing.js +++ b/packages/shared/forks/ReactFeatureFlags.testing.js @@ -50,6 +50,7 @@ export const decoupleUpdatePriorityFromScheduler = false; export const enableDiscreteEventFlushingChange = false; export const enableDoubleInvokingEffects = false; +export const enableUseRefMutationWarning = false; // Flow magic to verify the exports of this file match the original version. // eslint-disable-next-line no-unused-vars diff --git a/packages/shared/forks/ReactFeatureFlags.testing.www.js b/packages/shared/forks/ReactFeatureFlags.testing.www.js index 717199df1d5..ceee06e86eb 100644 --- a/packages/shared/forks/ReactFeatureFlags.testing.www.js +++ b/packages/shared/forks/ReactFeatureFlags.testing.www.js @@ -50,6 +50,7 @@ export const decoupleUpdatePriorityFromScheduler = false; export const enableDiscreteEventFlushingChange = true; export const enableDoubleInvokingEffects = false; +export const enableUseRefMutationWarning = false; // Flow magic to verify the exports of this file match the original version. // eslint-disable-next-line no-unused-vars diff --git a/packages/shared/forks/ReactFeatureFlags.www-dynamic.js b/packages/shared/forks/ReactFeatureFlags.www-dynamic.js index 103e8adc5c7..4a5b79bce69 100644 --- a/packages/shared/forks/ReactFeatureFlags.www-dynamic.js +++ b/packages/shared/forks/ReactFeatureFlags.www-dynamic.js @@ -48,3 +48,4 @@ export const enableTrustedTypesIntegration = false; export const disableSchedulerTimeoutBasedOnReactExpirationTime = false; export const enableDoubleInvokingEffects = false; +export const enableUseRefMutationWarning = false; diff --git a/packages/shared/forks/ReactFeatureFlags.www.js b/packages/shared/forks/ReactFeatureFlags.www.js index 58d6987495f..43b740d6526 100644 --- a/packages/shared/forks/ReactFeatureFlags.www.js +++ b/packages/shared/forks/ReactFeatureFlags.www.js @@ -28,6 +28,7 @@ export const { enableDebugTracing, skipUnmountedBoundaries, enableDoubleInvokingEffects, + enableUseRefMutationWarning, } = dynamicFeatureFlags; // On WWW, __EXPERIMENTAL__ is used for a new modern build. From 551985a9a6e227d6b03df19446abcbfc3779f982 Mon Sep 17 00:00:00 2001 From: Brian Vaughn Date: Mon, 19 Oct 2020 15:34:13 -0400 Subject: [PATCH 2/2] PR feedback MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit • Changed tests to use pragma • Renamed feature flag --- .../src/ReactFiberHooks.new.js | 4 +- .../src/ReactFiberHooks.old.js | 4 +- .../src/__tests__/useRef-test.internal.js | 59 ++++++++++--------- packages/shared/ReactFeatureFlags.js | 2 +- .../forks/ReactFeatureFlags.native-fb.js | 2 +- .../forks/ReactFeatureFlags.native-oss.js | 2 +- .../forks/ReactFeatureFlags.test-renderer.js | 2 +- .../ReactFeatureFlags.test-renderer.native.js | 2 +- .../ReactFeatureFlags.test-renderer.www.js | 2 +- .../shared/forks/ReactFeatureFlags.testing.js | 2 +- .../forks/ReactFeatureFlags.testing.www.js | 2 +- .../forks/ReactFeatureFlags.www-dynamic.js | 2 +- .../shared/forks/ReactFeatureFlags.www.js | 2 +- 13 files changed, 45 insertions(+), 42 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberHooks.new.js b/packages/react-reconciler/src/ReactFiberHooks.new.js index 9126ee1687a..231b33e69bc 100644 --- a/packages/react-reconciler/src/ReactFiberHooks.new.js +++ b/packages/react-reconciler/src/ReactFiberHooks.new.js @@ -27,7 +27,7 @@ import { enableNewReconciler, decoupleUpdatePriorityFromScheduler, enableDoubleInvokingEffects, - enableUseRefMutationWarning, + enableUseRefAccessWarning, } from 'shared/ReactFeatureFlags'; import { @@ -1216,7 +1216,7 @@ function getCallerStackFrame(): string { function mountRef(initialValue: T): {|current: T|} { const hook = mountWorkInProgressHook(); - if (enableUseRefMutationWarning) { + 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. diff --git a/packages/react-reconciler/src/ReactFiberHooks.old.js b/packages/react-reconciler/src/ReactFiberHooks.old.js index 826ae84cf76..ccd69abae6a 100644 --- a/packages/react-reconciler/src/ReactFiberHooks.old.js +++ b/packages/react-reconciler/src/ReactFiberHooks.old.js @@ -26,7 +26,7 @@ import { enableSchedulingProfiler, enableNewReconciler, decoupleUpdatePriorityFromScheduler, - enableUseRefMutationWarning, + enableUseRefAccessWarning, } from 'shared/ReactFeatureFlags'; import {NoMode, BlockingMode, DebugTracingMode} from './ReactTypeOfMode'; @@ -1194,7 +1194,7 @@ function getCallerStackFrame(): string { function mountRef(initialValue: T): {|current: T|} { const hook = mountWorkInProgressHook(); - if (enableUseRefMutationWarning) { + 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. diff --git a/packages/react-reconciler/src/__tests__/useRef-test.internal.js b/packages/react-reconciler/src/__tests__/useRef-test.internal.js index b072887a128..ccfc6d0d4b2 100644 --- a/packages/react-reconciler/src/__tests__/useRef-test.internal.js +++ b/packages/react-reconciler/src/__tests__/useRef-test.internal.js @@ -30,7 +30,6 @@ describe('useRef', () => { const ReactFeatureFlags = require('shared/ReactFeatureFlags'); ReactFeatureFlags.debugRenderPhaseSideEffectsForStrictMode = false; - ReactFeatureFlags.enableUseRefMutationWarning = true; act = ReactNoop.act; useCallback = React.useCallback; @@ -104,32 +103,6 @@ describe('useRef', () => { expect(Scheduler).toHaveYielded(['ping: 6']); }); - it('should never warn when attaching to children', () => { - class Component extends React.Component { - render() { - return null; - } - } - - function Example({phase}) { - const hostRef = useRef(); - const classRef = useRef(); - return ( - <> -
- - - ); - } - - act(() => { - ReactNoop.render(); - }); - act(() => { - ReactNoop.render(); - }); - }); - it('should return the same ref during re-renders', () => { function Counter() { const ref = useRef('val'); @@ -155,6 +128,33 @@ describe('useRef', () => { }); if (__DEV__) { + it('should never warn when attaching to children', () => { + class Component extends React.Component { + render() { + return null; + } + } + + function Example({phase}) { + const hostRef = useRef(); + const classRef = useRef(); + return ( + <> +
+ + + ); + } + + act(() => { + ReactNoop.render(); + }); + act(() => { + ReactNoop.render(); + }); + }); + + // @gate enableUseRefAccessWarning it('should warn about reads during render', () => { function Example() { const ref = useRef(123); @@ -215,7 +215,8 @@ describe('useRef', () => { }); }); - it('should not warn about unconditional lazy init during render', () => { + // @gate enableUseRefAccessWarning + it('should warn about unconditional lazy init during render', () => { function Example() { const ref1 = useRef(null); const ref2 = useRef(undefined); @@ -255,6 +256,7 @@ describe('useRef', () => { }); }); + // @gate enableUseRefAccessWarning it('should warn about reads to ref after lazy init pattern', () => { function Example() { const ref1 = useRef(null); @@ -288,6 +290,7 @@ describe('useRef', () => { }); }); + // @gate enableUseRefAccessWarning it('should warn about writes to ref after lazy init pattern', () => { function Example() { const ref1 = useRef(null); diff --git a/packages/shared/ReactFeatureFlags.js b/packages/shared/ReactFeatureFlags.js index 68da7c2f3a0..dbf9f5bd285 100644 --- a/packages/shared/ReactFeatureFlags.js +++ b/packages/shared/ReactFeatureFlags.js @@ -135,4 +135,4 @@ export const enableDiscreteEventFlushingChange = false; export const enableDoubleInvokingEffects = false; -export const enableUseRefMutationWarning = false; +export const enableUseRefAccessWarning = false; diff --git a/packages/shared/forks/ReactFeatureFlags.native-fb.js b/packages/shared/forks/ReactFeatureFlags.native-fb.js index 9ea390dc4cd..0df33470e57 100644 --- a/packages/shared/forks/ReactFeatureFlags.native-fb.js +++ b/packages/shared/forks/ReactFeatureFlags.native-fb.js @@ -51,7 +51,7 @@ export const decoupleUpdatePriorityFromScheduler = false; export const enableDiscreteEventFlushingChange = false; export const enableDoubleInvokingEffects = false; -export const enableUseRefMutationWarning = false; +export const enableUseRefAccessWarning = false; // Flow magic to verify the exports of this file match the original version. // eslint-disable-next-line no-unused-vars diff --git a/packages/shared/forks/ReactFeatureFlags.native-oss.js b/packages/shared/forks/ReactFeatureFlags.native-oss.js index b9b6072db6f..77148d9be29 100644 --- a/packages/shared/forks/ReactFeatureFlags.native-oss.js +++ b/packages/shared/forks/ReactFeatureFlags.native-oss.js @@ -50,7 +50,7 @@ export const decoupleUpdatePriorityFromScheduler = false; export const enableDiscreteEventFlushingChange = false; export const enableDoubleInvokingEffects = false; -export const enableUseRefMutationWarning = false; +export const enableUseRefAccessWarning = false; // Flow magic to verify the exports of this file match the original version. // eslint-disable-next-line no-unused-vars diff --git a/packages/shared/forks/ReactFeatureFlags.test-renderer.js b/packages/shared/forks/ReactFeatureFlags.test-renderer.js index 78470db3095..7e66eb9e75e 100644 --- a/packages/shared/forks/ReactFeatureFlags.test-renderer.js +++ b/packages/shared/forks/ReactFeatureFlags.test-renderer.js @@ -50,7 +50,7 @@ export const decoupleUpdatePriorityFromScheduler = false; export const enableDiscreteEventFlushingChange = false; export const enableDoubleInvokingEffects = false; -export const enableUseRefMutationWarning = false; +export const enableUseRefAccessWarning = false; // Flow magic to verify the exports of this file match the original version. // eslint-disable-next-line no-unused-vars diff --git a/packages/shared/forks/ReactFeatureFlags.test-renderer.native.js b/packages/shared/forks/ReactFeatureFlags.test-renderer.native.js index 132b37de777..90c4360c6d4 100644 --- a/packages/shared/forks/ReactFeatureFlags.test-renderer.native.js +++ b/packages/shared/forks/ReactFeatureFlags.test-renderer.native.js @@ -50,7 +50,7 @@ export const decoupleUpdatePriorityFromScheduler = false; export const enableDiscreteEventFlushingChange = false; export const enableDoubleInvokingEffects = false; -export const enableUseRefMutationWarning = false; +export const enableUseRefAccessWarning = false; // Flow magic to verify the exports of this file match the original version. // eslint-disable-next-line no-unused-vars diff --git a/packages/shared/forks/ReactFeatureFlags.test-renderer.www.js b/packages/shared/forks/ReactFeatureFlags.test-renderer.www.js index 9d5337dc8cb..4dbd671409e 100644 --- a/packages/shared/forks/ReactFeatureFlags.test-renderer.www.js +++ b/packages/shared/forks/ReactFeatureFlags.test-renderer.www.js @@ -50,7 +50,7 @@ export const decoupleUpdatePriorityFromScheduler = false; export const enableDiscreteEventFlushingChange = false; export const enableDoubleInvokingEffects = false; -export const enableUseRefMutationWarning = false; +export const enableUseRefAccessWarning = false; // Flow magic to verify the exports of this file match the original version. // eslint-disable-next-line no-unused-vars diff --git a/packages/shared/forks/ReactFeatureFlags.testing.js b/packages/shared/forks/ReactFeatureFlags.testing.js index 1ea9f969a1a..c98063376f0 100644 --- a/packages/shared/forks/ReactFeatureFlags.testing.js +++ b/packages/shared/forks/ReactFeatureFlags.testing.js @@ -50,7 +50,7 @@ export const decoupleUpdatePriorityFromScheduler = false; export const enableDiscreteEventFlushingChange = false; export const enableDoubleInvokingEffects = false; -export const enableUseRefMutationWarning = false; +export const enableUseRefAccessWarning = false; // Flow magic to verify the exports of this file match the original version. // eslint-disable-next-line no-unused-vars diff --git a/packages/shared/forks/ReactFeatureFlags.testing.www.js b/packages/shared/forks/ReactFeatureFlags.testing.www.js index ceee06e86eb..362569dfb01 100644 --- a/packages/shared/forks/ReactFeatureFlags.testing.www.js +++ b/packages/shared/forks/ReactFeatureFlags.testing.www.js @@ -50,7 +50,7 @@ export const decoupleUpdatePriorityFromScheduler = false; export const enableDiscreteEventFlushingChange = true; export const enableDoubleInvokingEffects = false; -export const enableUseRefMutationWarning = false; +export const enableUseRefAccessWarning = false; // Flow magic to verify the exports of this file match the original version. // eslint-disable-next-line no-unused-vars diff --git a/packages/shared/forks/ReactFeatureFlags.www-dynamic.js b/packages/shared/forks/ReactFeatureFlags.www-dynamic.js index 4a5b79bce69..d03ba3eadd4 100644 --- a/packages/shared/forks/ReactFeatureFlags.www-dynamic.js +++ b/packages/shared/forks/ReactFeatureFlags.www-dynamic.js @@ -48,4 +48,4 @@ export const enableTrustedTypesIntegration = false; export const disableSchedulerTimeoutBasedOnReactExpirationTime = false; export const enableDoubleInvokingEffects = false; -export const enableUseRefMutationWarning = false; +export const enableUseRefAccessWarning = __VARIANT__; diff --git a/packages/shared/forks/ReactFeatureFlags.www.js b/packages/shared/forks/ReactFeatureFlags.www.js index 43b740d6526..f5861a2e336 100644 --- a/packages/shared/forks/ReactFeatureFlags.www.js +++ b/packages/shared/forks/ReactFeatureFlags.www.js @@ -28,7 +28,7 @@ export const { enableDebugTracing, skipUnmountedBoundaries, enableDoubleInvokingEffects, - enableUseRefMutationWarning, + enableUseRefAccessWarning, } = dynamicFeatureFlags; // On WWW, __EXPERIMENTAL__ is used for a new modern build.