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

[josephsavona]

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]

Comparing: d5ddc6543efb2d26303a8bc3f38a5ad5dfe0e4f7...33e0f947516e491a82ba6aded1f702a4b539b7b0

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 :no_entry_sign: dangerJS against 33e0f947516e491a82ba6aded1f702a4b539b7b0

[github-actions[bot]]

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[bot]]

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!