Fix infinite update loop that happens when an unmemoized value is passed to useDeferredValue - #24247
Conversation
facebook-github-bot
added
CLA Signed
React Core Team
|
@bvaughn Do you have advice on what to do about react-debug-hooks? The internal hook layout has changed; there's only one internal hook object instead of two. Do I need to add a version check? |
|
Comparing: 0568c0f...e4fd270 Critical size changesIncludes critical production bundles, as well as any change greater than 2%:
Significant size changesIncludes any change greater than 0.2%: Expand to show
|
Either that– or I guess this is one of those unavoidable downside of our hooks-as-a-flat-list architecture. 😄 Speaking of which, I wonder if there wouldn't be something clever we could do to the hooks structure to make packages like [
// Only 1 hook with the batch ID signifies a simple hook e.g. useState, useReducer
{
batchID: 1,
memoizedState: null,
// ...
},
// 2 hooks sharing a batchID signifies a composite hook like useDeferredValue
{
batchID: 2,
memoizedState: null,
// ...
},
{
batchID: 2,
memoizedState: null,
// ...
}
]React itself doesn't need this info of course, so maybe adding it purely in the service of developer tooling isn't desirable, but it would enable |
We could definitely do something like that in dev/profiling at least, not sure if it's worth it for prod |
Right, that makes sense. Unfortunately it wouldn't be that helpful if we didn't also do it for production because |
|
What if React Debug Tools rendered each of built-in hooks by itself (just once during start up, perhaps, or the first time lazily) and counted how many internal hooks it produces? |
|
Seems kind of ok for DevTools to only support uDV in 18.0.1+. Given that it was only in one stable release. |
That's an interesting idea.
Probably okay in this specific case, but it seems likely to come up again with uSES or other composite hooks. |
I think this still might not work though, as React Debug Tools needs to know which internal hook stores the canonical value that should be returned to the component (to avoid the component trying to access an invalid property or something). For example, I think this happens to be the case for most "composite" hooks at the moment (e.g. |
dd45378 to
ab2d7b6
Compare
There was a problem hiding this comment.
Do they need to be the same one? Is that even desirable?
It’s unfortunate that we need another thread-local field. It’s getting to be a lot.
There was a problem hiding this comment.
Yeah maybe not, I think I thought maybe they should be batched together because in the previous implementation they were, but I think all that's important is that they don't cause a waterfall which the new implementation already accounts for
There was a problem hiding this comment.
Changed it to grab a new one each time
The current implementation of useDeferredValue will spawn a new render any time the input value is different from the previous one. So if you pass an unmemoized value (like an inline object), it will never stop spawning new renders. The fix is to only defer during an urgent render. If we're already inside a transition, retry, offscreen, or other non-urgen render, then we can use the latest value.
DevTools' timeline profiler warns if an update inside a layout effect results in an expensive re-render. However, it misattributes renders that are spawned from a sync render at lower priority. This affects the new implementation of useDeferredValue but it would also apply to things like Offscreen. It's not obvious to me how to fix this given how DevTools models the idea of a "nested update" so I'm disabling the warning for now to unblock the bugfix for useDeferredValue.
ab2d7b6 to
e4fd270
Compare
7 participants
The current implementation of useDeferredValue will spawn a new render any time the input value is different from the previous one. So if you pass an unmemoized value (like an inline object), it will never stop spawning new renders.
The fix is to only defer during an urgent render. If we're already inside a transition, retry, offscreen, or other non-urgent render, then we can use the latest value.