-
Notifications
You must be signed in to change notification settings - Fork 45.5k
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
Invariant that throws when committing wrong tree #15517
Conversation
If React finishes rendering a tree, delays committing it (e.g. Suspense), then subsequently starts over or renders a new tree, the pending tree is no longer valid. That's because rendering a new work-in progress mutates the old one in place. The current structure of the work loop makes this hard to reason about because, although `renderRoot` and `commitRoot` are separate functions, they can't be interleaved. If they are interleaved by accident, it either results in inconsistent render output or invariant violations that are hard to debug. This commit adds an invariant that throws if the new tree is the same as the old one. This won't prevent all bugs of this class, but it should catch the most common kind. To implement the invariant, I store the finished tree on a field on the root. We already had a field for this, but it was only being used for the unstable `createBatch` feature. A more rigorous way to address this type of problem could be to unify `renderRoot` and `commitRoot` into a single function, so that it's harder to accidentally interleave the two phases. I plan to do something like this in a follow-up.
ReactDOM: size: 🔺+0.1%, gzip: 🔺+0.1% Details of bundled changes.Comparing: c4d1dcb...a592c8a react-dom
react-art
react-native-renderer
react-reconciler
Generated by 🚫 dangerJS |
Was it expected to actually fix #15512? I still see the number glitching but with this change, I can't get neither the invariant nor the warning to trigger anymore. |
It fixes it but I didn’t isolate it to a test case yet. I know what’s happening at a high level but I don’t know what specific sequence of events needs to happen to trigger it. I’ll look into it more on Monday. |
If React finishes rendering a tree, delays committing it (e.g. Suspense), then subsequently starts over or renders a new tree, the pending tree is no longer valid. That's because rendering a new work-in progress mutates the old one in place.
The current structure of the work loop makes this hard to reason about because, although
renderRoot
andcommitRoot
are separate functions, they can't be interleaved. If they are interleaved by accident, it either results in inconsistent render output or invariant violations that are hard to debug.This commit adds an invariant that throws if the new tree is the same as the old one. This won't prevent all bugs of this class, but it should catch the most common kind.
To implement the invariant, I store the finished tree on a field on the root. We already had a field for this, but it was only being used for the unstable
createBatch
feature.A more rigorous way to address this type of problem could be to unify
renderRoot
andcommitRoot
into a single function, so that it's harder to accidentally interleave the two phases. I plan to do something like this in a follow-up.