[suspense] Avoid double commit by re-rendering immediately and reusing primary children #14083
Conversation
acdlite
force-pushed
the
avoid-double-commit-outside-concurrent
branch
from
3ba3f21
to
7432297
Compare
|
ReactDOM: size: -0.2%, gzip: -0.7% Details of bundled changes.Comparing: 9d47143...d046c96 react-dom
react-art
react-test-renderer
react-reconciler
react-native-renderer
scheduler
Generated by 🚫 dangerJS |
acdlite
force-pushed
the
avoid-double-commit-outside-concurrent
branch
from
7432297
to
ea3739e
Compare
|
Would this also happen to fix #14079 (comment) since you removed that field? |
|
@gaearon Maybe! I'll check later, time for me to go home. |
acdlite
force-pushed
the
avoid-double-commit-outside-concurrent
branch
3 times, most recently
from
1815355
to
f24d201
Compare
|
@gaearon Yep it fixes it |
|
Why is this fixing the bugs? Seems like we should be able to fix the bugs without changing the semantics? I’m not sure these semantics make more sense. Both semantics are slightly broken in various cases. Suspense doesn’t fully make sense without ConcurrentMode. It’s always slightly broken. Why is this brokenness better? It seems like the worst of both worlds because it doesn’t give the correct semantics in the suspended tree (it renders null even when the component never does) and it gives the incorrect semantics in the suspense component because it does two passes which is not possible in other cases so we’d have to think what other consequences that causes. |
It fixes two bugs. The first one, where React fails to switch back to the fallback even after a promise resolves, was just a mistake. I could split that into a separate PR if you like. (I wasn't even intending to fix that bug in this PR — I just happened to fix it in the course of refactoring.) The other bug is that the fallback is always deleted on every ping. That one can't be fixed without changing semantics.
Two passes is how it works in concurrent mode, too. It's also how error boundaries work, both in and out of concurrent mode. The difference is that instead of reusing the current primary tree, we commit it in an inconsistent state and hide it. I'd argue this is less of a deviation from the concurrent version; the only difference now is whether use the incomplete work-in-progress tree or if we throw it out and revert to the current one. As supporting evidence for why the updated model is simpler, note that the changes to the SuspenseState type. We can now track fewer pieces of state because everything happens within a single render phase: https://github.com/facebook/react/pull/14083/files#diff-2c5a7d418c1fe3bfba84500fc7231482 |
|
Also consider if I were refactoring in the opposite direction. The previous semantics make less sense — for one, because it forces us to delete and remount the fallback on every ping that doesn't completely resolve the tree. But also because it's unnecessary. We can do everything within a single render phase. The only reason we used to not be able to do that is because 1) we needed to commit the primary tree in order to fire the effects, for compatibility outside strict mode 2) we needed to delete the tree in a subsequent, sync commit to prevent the user from seeing an inconsistent state. Since #13823, though, we don't do step 2) anymore. Instead of deleting the inconsistent tree, we hide it, so that we can preserve the state. So if we were deciding on the least-bad non-concurrent mode semantics to use today, there's no way we'd choose the model that's currently in master. |
The goal here is to deviate from sync mode as little as possible. Not to bring it closer to concurrent mode. The goal is to make it (almost) completely safe to adopt this in sync mode. It's a non-goal to make the sync mode the best that it can be - if that is causing further risk of breakages during adoption of Suspense. |
|
@sebmarkbage I feel like you're complaining about change in the abstract instead of addressing anything I've actually changed. Can you comment on remounting the fallback on every ping specifically? That's the crux of this PR. |
|
My issue is that I feel like you haven't addressed the opposite tradeoff. What sync code will this semantic break? |
|
Chatted offline, here's the summary:
Follow-up items:
|
|
Thanks for posting the summary. This is really helpful for understanding. |
To support Suspense outside of concurrent mode, any component that starts rendering must commit synchronously without being interrupted. This means normal path, where we unwind the stack and try again from the nearest Suspense boundary, won't work. We used to have a special case where we commit the suspended tree in an incomplete state. Then, in a subsequent commit, we re-render using the fallback. The first part — committing an incomplete tree — hasn't changed with this PR. But I've changed the second part — now we render the fallback children immediately, within the same commit.
If parent reads visibility of children in a lifecycle, they should have already updated.
acdlite
force-pushed
the
avoid-double-commit-outside-concurrent
branch
from
d0982cc
to
d046c96
Compare
…g primary children (facebook#14083) * Avoid double commit by re-rendering immediately and reusing children To support Suspense outside of concurrent mode, any component that starts rendering must commit synchronously without being interrupted. This means normal path, where we unwind the stack and try again from the nearest Suspense boundary, won't work. We used to have a special case where we commit the suspended tree in an incomplete state. Then, in a subsequent commit, we re-render using the fallback. The first part — committing an incomplete tree — hasn't changed with this PR. But I've changed the second part — now we render the fallback children immediately, within the same commit. * Add a failing test for remounting fallback in sync mode * Add failing test for stuck Suspense fallback * Toggle visibility of Suspense children in mutation phase, not layout If parent reads visibility of children in a lifecycle, they should have already updated.
…g primary children (facebook#14083) * Avoid double commit by re-rendering immediately and reusing children To support Suspense outside of concurrent mode, any component that starts rendering must commit synchronously without being interrupted. This means normal path, where we unwind the stack and try again from the nearest Suspense boundary, won't work. We used to have a special case where we commit the suspended tree in an incomplete state. Then, in a subsequent commit, we re-render using the fallback. The first part — committing an incomplete tree — hasn't changed with this PR. But I've changed the second part — now we render the fallback children immediately, within the same commit. * Add a failing test for remounting fallback in sync mode * Add failing test for stuck Suspense fallback * Toggle visibility of Suspense children in mutation phase, not layout If parent reads visibility of children in a lifecycle, they should have already updated.
…g primary children (facebook#14083) * Avoid double commit by re-rendering immediately and reusing children To support Suspense outside of concurrent mode, any component that starts rendering must commit synchronously without being interrupted. This means normal path, where we unwind the stack and try again from the nearest Suspense boundary, won't work. We used to have a special case where we commit the suspended tree in an incomplete state. Then, in a subsequent commit, we re-render using the fallback. The first part — committing an incomplete tree — hasn't changed with this PR. But I've changed the second part — now we render the fallback children immediately, within the same commit. * Add a failing test for remounting fallback in sync mode * Add failing test for stuck Suspense fallback * Toggle visibility of Suspense children in mutation phase, not layout If parent reads visibility of children in a lifecycle, they should have already updated.
To support Suspense outside of concurrent mode, any component that starts rendering must commit synchronously without being interrupted. This means the normal path, where we unwind the stack and try again from the nearest Suspense boundary, won't work.
We used to have a special case where we commit the suspended tree in an incomplete state. Then, in a subsequent commit, we re-render using the fallback.
The first part — committing an incomplete tree — hasn't changed with this PR. But I've changed the second part — now we render the fallback children immediately, within the same commit.
Fixes #13999
Fixes #14013
Fixes #14073
Fixes #14078
Fixes #14079