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.
This commit is contained in:
Brian Vaughn
2020-10-19 16:05:00 -04:00
committed by GitHub
parent 75726fadfd
commit c59c3dfe55
15 changed files with 567 additions and 121 deletions
@@ -474,12 +474,12 @@ 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(
@@ -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 <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');
},
);
});
@@ -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];
}
@@ -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];
}
@@ -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(<App />);
});
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 <Text text={ref.current} />;
}
ReactNoop.render(<Counter />);
expect(Scheduler).toFlushAndYield(['val']);
ReactNoop.render(<Counter />);
expect(Scheduler).toFlushAndYield(['val']);
});
});
describe('useImperativeHandle', () => {
it('does not update when deps are the same', () => {
const INCREMENT = 'INCREMENT';
@@ -0,0 +1,364 @@
/**
* 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;
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 <span prop={props.text} />;
}
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(<App />);
});
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 <Text text={count} />;
}
ReactNoop.render(<Counter />);
expect(Scheduler).toFlushAndYield([3]);
ReactNoop.render(<Counter />);
expect(Scheduler).toFlushAndYield([3]);
});
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 (
<>
<div key={`host-${phase}`} ref={hostRef} />
<Component key={`class-${phase}`} ref={classRef} />
</>
);
}
act(() => {
ReactNoop.render(<Example phase="mount" />);
});
act(() => {
ReactNoop.render(<Example phase="update" />);
});
});
// @gate enableUseRefAccessWarning
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(<Example />);
});
});
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(<Example />);
});
// Should not warn after an update either.
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(undefined);
useLayoutEffect(() => {
ref1.current = 123;
ref2.current = 123;
setDidMount(true);
}, []);
return null;
}
act(() => {
ReactNoop.render(<Example />);
});
});
// @gate enableUseRefAccessWarning
it('should 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(<Example />);
});
// Should not warn again on update.
shouldExpectWarning = false;
act(() => {
ReactNoop.render(<Example />);
});
});
// @gate enableUseRefAccessWarning
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(<Example />);
});
});
// @gate enableUseRefAccessWarning
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(<Example />);
});
});
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(<Example />);
});
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(<Example />);
});
expect(ref.current).toBe(123);
ref.current = 456;
expect(ref.current).toBe(456);
});
}
});
+2
View File
@@ -134,3 +134,5 @@ export const decoupleUpdatePriorityFromScheduler = false;
export const enableDiscreteEventFlushingChange = false;
export const enableDoubleInvokingEffects = false;
export const enableUseRefAccessWarning = false;
@@ -51,6 +51,7 @@ export const decoupleUpdatePriorityFromScheduler = false;
export const enableDiscreteEventFlushingChange = false;
export const enableDoubleInvokingEffects = 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
@@ -50,6 +50,7 @@ export const decoupleUpdatePriorityFromScheduler = false;
export const enableDiscreteEventFlushingChange = false;
export const enableDoubleInvokingEffects = 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
@@ -50,6 +50,7 @@ export const decoupleUpdatePriorityFromScheduler = false;
export const enableDiscreteEventFlushingChange = false;
export const enableDoubleInvokingEffects = 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
@@ -50,6 +50,7 @@ export const decoupleUpdatePriorityFromScheduler = false;
export const enableDiscreteEventFlushingChange = false;
export const enableDoubleInvokingEffects = 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
@@ -50,6 +50,7 @@ export const decoupleUpdatePriorityFromScheduler = false;
export const enableDiscreteEventFlushingChange = false;
export const enableDoubleInvokingEffects = 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
@@ -50,6 +50,7 @@ export const decoupleUpdatePriorityFromScheduler = false;
export const enableDiscreteEventFlushingChange = false;
export const enableDoubleInvokingEffects = 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
@@ -50,6 +50,7 @@ export const decoupleUpdatePriorityFromScheduler = false;
export const enableDiscreteEventFlushingChange = true;
export const enableDoubleInvokingEffects = 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
@@ -48,3 +48,4 @@ export const enableTrustedTypesIntegration = false;
export const disableSchedulerTimeoutBasedOnReactExpirationTime = false;
export const enableDoubleInvokingEffects = false;
export const enableUseRefAccessWarning = __VARIANT__;
@@ -28,6 +28,7 @@ export const {
enableDebugTracing,
skipUnmountedBoundaries,
enableDoubleInvokingEffects,
enableUseRefAccessWarning,
} = dynamicFeatureFlags;
// On WWW, __EXPERIMENTAL__ is used for a new modern build.