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.

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

Test case for potentially undesirable combo of useMemo w setState-in-render #25227

Closed

Conversation

josephsavona
Copy link
Contributor

@josephsavona josephsavona commented Sep 9, 2022

Summary

This came out of an offline discussion about useMemoCache() (#25143) and whether, after a setState in render, the cache should reset back to that of the current fiber or continue using the wip cache. @acdlite pointed out that useMemo() does the equivalent of the latter, ie reusing the wip memo cache, but I realized that this can break memoization of child components in some edge cases (ie, cause child components to rerender/recompute memoized values due to unnecessarily breaking referential equality in the parent).

A render pass that does not contain a setState will always compute useMemo values relative to the current fiber: this means that if the dependencies have not changed, the computed memo value will be the same as the object that was computed and passed to children (and that thefore is present as the dependency value in their useMemo deps arrays):

  • Parent renders with input@v0, computes value@v0
  • Parent updates with input@v0, reuses value@v0
    • downstream memo caches don't recompute bc their input is the same

However, a render pass that does have a setState will (currently) reuse the work-in-progress useMemo values. Even if the input is reset to its last committed value, dependencies have already been reset:

  • Parent renders with input@v0, computes value@v0
  • Parent updates with input@v1, computes value@v1 (at this point the value@v0 is lost). SetState resets back to input@v0
  • Parent updates with input@v0, recomputes value@v2 (which is structurally equal, but !== to, value@v0
    • oops, downstream memo caches recompute bc their input is accidentally different

The current behavior is in some senses reasonable: a setState in render that happens to reset back to the same state as the last commit seems very unlikely in practice, whereas a setState in render having to redo the same (new) computation again seems likely. However, recomputing a single new value an extra time is less costly than breaking memoization caches for an entire subtree, so it's worth considering.

How did you test this change?

yarn test (the new test is expected to fail)

@sizebot
Copy link

sizebot commented Sep 9, 2022

Comparing: d5ddc65...33e0f94

Critical size changes

Includes critical production bundles, as well as any change greater than 2%:

Name +/- Base Current +/- gzip Base gzip Current gzip
oss-stable/react-dom/cjs/react-dom.production.min.js = 134.97 kB 134.97 kB = 43.23 kB 43.23 kB
oss-experimental/react-dom/cjs/react-dom.production.min.js = 141.69 kB 141.69 kB = 45.23 kB 45.23 kB
facebook-www/ReactDOM-prod.classic.js = 486.24 kB 486.24 kB = 86.55 kB 86.55 kB
facebook-www/ReactDOM-prod.modern.js = 471.52 kB 471.52 kB = 84.32 kB 84.32 kB
facebook-www/ReactDOMForked-prod.classic.js = 486.24 kB 486.24 kB = 86.55 kB 86.55 kB

Significant size changes

Includes any change greater than 0.2%:

(No significant changes)

Generated by 🚫 dangerJS against 33e0f94

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
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 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.

Copy link

This pull request has been automatically marked as stale. If this pull request is still relevant, please leave any comment (for example, "bump"), and we'll keep it open. We are sorry that we haven't been able to prioritize reviewing it yet. Your contribution is very much appreciated.

@github-actions github-actions bot added the Resolution: Stale Automatically closed due to inactivity label Apr 10, 2024
Copy link

Closing this pull request after a prolonged period of inactivity. If this issue is still present in the latest release, please ask for this pull request to be reopened. Thank you!

@github-actions github-actions bot closed this Apr 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed Resolution: Stale Automatically closed due to inactivity
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants