Skip to content

Commit

Permalink
useRef: Warn if reading mutable value during render
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
Brian Vaughn committed Apr 8, 2020
1 parent 5b1d0f0 commit ee03c74
Show file tree
Hide file tree
Showing 3 changed files with 136 additions and 3 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -503,6 +503,9 @@ describe('ReactDOMServerHooks', () => {
return <span>Count: {ref.current}</span>;
}

// Reading from ref during render (after a mutation) triggers a warning.
spyOnDev(console, 'warn');

const domNode = await render(<Counter />);
expect(clearYields()).toEqual([0, 1, 2, 3]);
expect(domNode.textContent).toEqual('Count: 3');
Expand Down
30 changes: 27 additions & 3 deletions packages/react-reconciler/src/ReactFiberHooks.js
Original file line number Diff line number Diff line change
Expand Up @@ -1233,12 +1233,36 @@ function pushEffect(tag, create, destroy, deps) {

function mountRef<T>(initialValue: T): {|current: T|} {
const hook = mountWorkInProgressHook();
const ref = {current: initialValue};
if (__DEV__) {
let current = initialValue;
let shouldWarnAboutRead = false;
const ref = {
get current() {
if (shouldWarnAboutRead && currentlyRenderingFiber !== null) {
console.warn(
'useRef: 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.',
);
}
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<T>(initialValue: T): {|current: T|} {
Expand Down
106 changes: 106 additions & 0 deletions packages/react-reconciler/src/__tests__/useRef-test.internal.js
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ describe('useRef', () => {
let act;
let useCallback;
let useEffect;
let useLayoutEffect;
let useRef;
let useState;

Expand All @@ -33,6 +34,7 @@ describe('useRef', () => {
act = ReactNoop.act;
useCallback = React.useCallback;
useEffect = React.useEffect;
useLayoutEffect = React.useLayoutEffect;
useRef = React.useRef;
useState = React.useState;
});
Expand Down Expand Up @@ -124,4 +126,108 @@ describe('useRef', () => {
ReactNoop.render(<Counter />);
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(<Example />);
});
});

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(['useRef: Unsafe read of a mutable ref during render.']);

return value;
}

act(() => {
ReactNoop.render(<Example />);
});
});

it('should not warn about lazy init pattern', () => {
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(<Example />);
});
});

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(['useRef: Unsafe read of a mutable ref during render.']);

return null;
}

act(() => {
ReactNoop.render(<Example />);
});
});

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(<Example />);
});

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(<Example />);
});

expect(ref.current).toBe(456);
});
}
});

0 comments on commit ee03c74

Please sign in to comment.