-
Notifications
You must be signed in to change notification settings - Fork 49k
Test case for potentially undesirable combo of useMemo w setState-in-render #25227
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.
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
Closed
Closed
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
89 changes: 89 additions & 0 deletions
89
packages/react-reconciler/src/__tests__/useMemo-invalidation-test.js
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,89 @@ | ||
let React; | ||
let ReactNoop; | ||
let act; | ||
let useState; | ||
|
||
describe('possible useMemo invalidation bug', () => { | ||
beforeEach(() => { | ||
jest.resetModules(); | ||
|
||
React = require('react'); | ||
ReactNoop = require('react-noop-renderer'); | ||
act = require('jest-react').act; | ||
useState = React.useState; | ||
}); | ||
|
||
// @gate experimental || www | ||
test('caches values whose inputs are unchanged after setstate in render (useMemo)', async () => { | ||
let setX; | ||
function Component({limit}) { | ||
const [x, _setX] = useState(0); | ||
setX = _setX; | ||
|
||
// `x` is a controlled state that can't be set higher than the provided `limit` | ||
if (x > limit) { | ||
setX(limit); | ||
} | ||
|
||
// `obj` is an object that captures the value of `x` | ||
const obj = React.useMemo( | ||
// POSSIBLE BUG: because useMemo tries to reuse the WIP memo value after a setState-in-render, | ||
// the previous value is discarded. This means that even though the final render has the same | ||
// inputs as the last commit, the memoized value is discarded and recreated, breaking | ||
// memoization of all the child components down the tree. | ||
// | ||
// 1) First render: cache is initialized to {count: 0} with deps of [x=0, limit=10] | ||
// 2) Update: cache updated to {count: 10} with deps of [x=10, limit=10] | ||
// 3) Second update: | ||
// 3a) initially renders and caches {count: 12} with deps of [x=12, limit=10] | ||
// 3b) re-renders bc of setstate, caches {count: 10} with deps of [x=10, limit=10] | ||
// If this last step started from the `current` fiber's memo cache (from 2, instead of from 3a), | ||
// then it would not re-execute and preserve object identity | ||
() => { | ||
return {count: x}; | ||
}, | ||
// Note that `limit` isn't technically a dependency, | ||
// it's included here to show that even if we modeled that | ||
// `x` can depend on the value of `limit`, it isn't sufficient | ||
// to avoid breaking memoization across renders | ||
[x, limit], | ||
); | ||
|
||
return <Child obj={obj} />; | ||
} | ||
|
||
const Child = React.memo(function Child({obj}) { | ||
const text = React.useMemo(() => { | ||
return {text: `Count ${obj.count}`}; | ||
}, [obj]); | ||
return <Text value={text} />; | ||
}); | ||
|
||
// Text should only ever re-render if the object identity of `value` | ||
// changes. | ||
let renderCount = 0; | ||
const Text = React.memo(function Text({value}) { | ||
renderCount++; | ||
return value.text; | ||
}); | ||
|
||
const root = ReactNoop.createRoot(); | ||
await act(async () => { | ||
root.render(<Component limit={10} />); | ||
}); | ||
expect(root).toMatchRenderedOutput('Count 0'); | ||
expect(renderCount).toBe(1); | ||
|
||
await act(async () => { | ||
setX(10); // set to the limit | ||
}); | ||
expect(root).toMatchRenderedOutput('Count 10'); | ||
expect(renderCount).toBe(2); | ||
|
||
await act(async () => { | ||
setX(12); // exceeds limit, will be reset in setState during render | ||
}); | ||
expect(root).toMatchRenderedOutput('Count 10'); | ||
expect(renderCount).toBe(2); // should not re-render, since value has not changed | ||
}); | ||
}); |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This fails with
renderCount === 3
. I implemented a version of this test on top of useMemoCache from #25143 (where useMemoCache does reset to the current fiber) and the test passes. If i change useMemoCache to reuse the wip cache as useMemo does, it also fails with renderCount=3.