Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 2 commits into from
Oct 19, 2020
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
32e5c1b
useRef: Warn about reading or writing mutable values during render
Oct 18, 2020
551985a
PR feedback
Oct 19, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 8 additions & 10 deletions packages/react-dom/src/__tests__/ReactDOMServerIntegrationHooks-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 />);
Expand All @@ -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) {
Expand All @@ -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');
},
);
});
Expand Down
103 changes: 91 additions & 12 deletions packages/react-reconciler/src/ReactFiberHooks.new.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ import {
enableNewReconciler,
decoupleUpdatePriorityFromScheduler,
enableDoubleInvokingEffects,
enableUseRefAccessWarning,
} from 'shared/ReactFeatureFlags';

import {
Expand Down Expand Up @@ -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__) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This nesting is to satisfy our no-logging-in-production lint rule.

// 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|} {
Expand Down Expand Up @@ -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];
}

Expand Down
103 changes: 91 additions & 12 deletions packages/react-reconciler/src/ReactFiberHooks.old.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ import {
enableSchedulingProfiler,
enableNewReconciler,
decoupleUpdatePriorityFromScheduler,
enableUseRefAccessWarning,
} from 'shared/ReactFeatureFlags';

import {NoMode, BlockingMode, DebugTracingMode} from './ReactTypeOfMode';
Expand Down Expand Up @@ -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;
bvaughn marked this conversation as resolved.
Show resolved Hide resolved
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) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if it's read inside a class component? Or written to. Should it warn?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Undefined I guess. I could expand the warning to include class components as well, if you think that's worth doing.

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|} {
Expand Down Expand Up @@ -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];
}

Expand Down
Loading