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

[suspense] Avoid double commit by re-rendering immediately and reusing primary children #14083

Merged

Conversation

acdlite
Copy link
Collaborator

@acdlite acdlite commented Nov 3, 2018

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

@acdlite acdlite force-pushed the avoid-double-commit-outside-concurrent branch from 3ba3f21 to 7432297 Compare November 3, 2018 03:10
@sizebot
Copy link

sizebot commented Nov 3, 2018

ReactDOM: size: -0.2%, gzip: -0.7%

Details of bundled changes.

Comparing: 9d47143...d046c96

react-dom

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-dom.development.js +0.1% 0.0% 704.36 KB 705.16 KB 163.31 KB 163.35 KB UMD_DEV
react-dom.production.min.js -0.2% -0.7% 98.32 KB 98.08 KB 32.11 KB 31.9 KB UMD_PROD
react-dom.development.js +0.1% 0.0% 699.67 KB 700.47 KB 161.94 KB 161.97 KB NODE_DEV
react-dom.production.min.js -0.3% -0.6% 98.37 KB 98.08 KB 31.63 KB 31.44 KB NODE_PROD
ReactDOM-dev.js +0.2% +0.1% 719.13 KB 720.29 KB 162.96 KB 163.06 KB FB_WWW_DEV
ReactDOM-prod.js 🔺+0.2% -0.4% 295.96 KB 296.42 KB 54.43 KB 54.24 KB FB_WWW_PROD
react-dom.profiling.min.js +0.1% -0.1% 100.73 KB 100.84 KB 31.98 KB 31.96 KB NODE_PROFILING
ReactDOM-profiling.js +0.8% +0.3% 300.09 KB 302.38 KB 55.43 KB 55.57 KB FB_WWW_PROFILING
react-dom.profiling.min.js +0.1% -0.0% 100.64 KB 100.75 KB 32.51 KB 32.5 KB UMD_PROFILING

react-art

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-art.development.js +0.2% 0.0% 490.96 KB 491.79 KB 108.88 KB 108.91 KB UMD_DEV
react-art.production.min.js 🔺+0.1% -0.1% 90.11 KB 90.2 KB 27.69 KB 27.65 KB UMD_PROD
react-art.development.js +0.2% 0.0% 422.75 KB 423.56 KB 91.87 KB 91.89 KB NODE_DEV
react-art.production.min.js 🔺+0.1% -0.1% 55.12 KB 55.18 KB 16.99 KB 16.98 KB NODE_PROD
ReactART-dev.js +0.3% +0.1% 428.55 KB 429.71 KB 90.57 KB 90.67 KB FB_WWW_DEV
ReactART-prod.js 🔺+0.6% 🔺+0.1% 168.92 KB 170 KB 28.59 KB 28.61 KB FB_WWW_PROD

react-test-renderer

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-test-renderer.development.js +0.2% 0.0% 435.42 KB 436.22 KB 94.55 KB 94.57 KB UMD_DEV
react-test-renderer.production.min.js 🔺+0.2% -0.6% 56.31 KB 56.4 KB 17.35 KB 17.24 KB UMD_PROD
react-test-renderer.development.js +0.2% 0.0% 430.63 KB 431.44 KB 93.4 KB 93.43 KB NODE_DEV
react-test-renderer.production.min.js 🔺+0.1% -0.2% 55.99 KB 56.07 KB 17.15 KB 17.11 KB NODE_PROD
ReactTestRenderer-dev.js +0.3% +0.1% 436.62 KB 437.78 KB 92.43 KB 92.52 KB FB_WWW_DEV

react-reconciler

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-reconciler.development.js +0.2% 0.0% 420.6 KB 421.41 KB 90.31 KB 90.34 KB NODE_DEV
react-reconciler.production.min.js 0.0% -0.2% 56.32 KB 56.32 KB 16.87 KB 16.84 KB NODE_PROD
react-reconciler-persistent.development.js +0.2% 0.0% 419.05 KB 419.85 KB 89.68 KB 89.72 KB NODE_DEV
react-reconciler-persistent.production.min.js 0.0% -0.2% 56.33 KB 56.33 KB 16.87 KB 16.84 KB NODE_PROD

react-native-renderer

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
ReactNativeRenderer-dev.js +0.2% +0.1% 554.28 KB 555.44 KB 121.37 KB 121.46 KB RN_FB_DEV
ReactNativeRenderer-prod.js 🔺+0.3% 0.0% 239.67 KB 240.27 KB 42.26 KB 42.28 KB RN_FB_PROD
ReactNativeRenderer-dev.js +0.2% +0.1% 553.98 KB 555.14 KB 121.27 KB 121.37 KB RN_OSS_DEV
ReactNativeRenderer-prod.js 🔺+0.3% 🔺+0.2% 225.15 KB 225.72 KB 39.17 KB 39.24 KB RN_OSS_PROD
ReactFabric-dev.js +0.2% +0.1% 544.59 KB 545.76 KB 118.88 KB 118.98 KB RN_FB_DEV
ReactFabric-prod.js 🔺+0.4% 🔺+0.1% 219 KB 219.97 KB 37.87 KB 37.91 KB RN_FB_PROD
ReactFabric-dev.js +0.2% +0.1% 544.63 KB 545.79 KB 118.9 KB 119 KB RN_OSS_DEV
ReactFabric-prod.js 🔺+0.4% 🔺+0.1% 219.04 KB 220 KB 37.88 KB 37.92 KB RN_OSS_PROD
ReactNativeRenderer-profiling.js +0.5% -0.0% 229.84 KB 230.89 KB 40.43 KB 40.42 KB RN_OSS_PROFILING
ReactFabric-profiling.js +0.4% 0.0% 223.35 KB 224.26 KB 39.14 KB 39.15 KB RN_OSS_PROFILING
ReactNativeRenderer-profiling.js +0.4% +0.2% 244.62 KB 245.7 KB 43.4 KB 43.5 KB RN_FB_PROFILING
ReactFabric-profiling.js +0.4% 0.0% 223.31 KB 224.22 KB 39.13 KB 39.14 KB RN_FB_PROFILING

scheduler

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
scheduler.development.js n/a n/a 0 B 19.17 KB 0 B 5.74 KB UMD_DEV
scheduler.production.min.js n/a n/a 0 B 3.16 KB 0 B 1.53 KB UMD_PROD

Generated by 🚫 dangerJS

@acdlite acdlite force-pushed the avoid-double-commit-outside-concurrent branch from 7432297 to ea3739e Compare November 3, 2018 03:16
@gaearon
Copy link
Collaborator

gaearon commented Nov 3, 2018

Would this also happen to fix #14079 (comment) since you removed that field?

@acdlite
Copy link
Collaborator Author

acdlite commented Nov 3, 2018

@gaearon Maybe! I'll check later, time for me to go home.

@acdlite acdlite force-pushed the avoid-double-commit-outside-concurrent branch 3 times, most recently from 1815355 to f24d201 Compare November 4, 2018 23:03
@acdlite
Copy link
Collaborator Author

acdlite commented Nov 4, 2018

@gaearon Yep it fixes it

@sebmarkbage
Copy link
Collaborator

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.

@acdlite
Copy link
Collaborator Author

acdlite commented Nov 5, 2018

Why is this fixing the bugs? Seems like we should be able to fix the bugs without changing the semantics?

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.

It’s always slightly broken. Why is this brokenness better?

it does two passes which is not possible in other cases so we’d have to think what other consequences that causes.

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

@acdlite
Copy link
Collaborator Author

acdlite commented Nov 5, 2018

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.

@sebmarkbage
Copy link
Collaborator

sebmarkbage commented Nov 5, 2018

I'd argue this is less of a deviation from the concurrent version

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.

@acdlite
Copy link
Collaborator Author

acdlite commented Nov 5, 2018

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

@sebmarkbage
Copy link
Collaborator

My issue is that I feel like you haven't addressed the opposite tradeoff. What sync code will this semantic break?

@acdlite
Copy link
Collaborator Author

acdlite commented Nov 5, 2018

Chatted offline, here's the summary:

  • The key change in this PR is to render both sets of children — the suspended children and the fallback children — within a single commit. This fixes the issue where the fallback is deleted and remounted on every synchronous ping.
  • The PR also happens to fix another bug (Suspense doesn't stop rendering fallback after Lazy resolves #13999 and duplicates) but that was an accident. I should have made this clearer in the PR description.
  • The conceptual model (in non-concurrent mode) is that we always render both sets of children. Then, in the commit phase, we hide either the primary set or the fallback set depending on whether something in the primary set suspended.
  • In our actual implementation, we cheat a bit in the non-suspended case by rendering nothing in place of the fallback, since we know it won't be needed.

Follow-up items:

  • Should toggle the visibility of the children in the mutation phase, not the layout phase. That way if any parents in between the Suspense boundary and the host children read the children in a lifecycle, their visibility has already been updated.

@gaearon
Copy link
Collaborator

gaearon commented Nov 5, 2018

Thanks for posting the summary. This is really helpful for understanding.

gaearon and others added 4 commits November 5, 2018 16:24
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 acdlite force-pushed the avoid-double-commit-outside-concurrent branch from d0982cc to d046c96 Compare November 6, 2018 00:25
@acdlite acdlite merged commit e9a2ec9 into facebook:master Nov 6, 2018
gaearon pushed a commit to gaearon/react that referenced this pull request Nov 6, 2018
…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.
jetoneza pushed a commit to jetoneza/react that referenced this pull request Jan 23, 2019
…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.
n8schloss pushed a commit to n8schloss/react that referenced this pull request Jan 31, 2019
…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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants