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

[Synchronous Suspense] Don't delete children of suspended component #14157

Merged

Conversation

acdlite
Copy link
Collaborator

@acdlite acdlite commented Nov 8, 2018

Vestigial behavior that should have been removed in #13823.

Found using the Suspense fuzz tester in #14147.

Copy link
Collaborator

@sebmarkbage sebmarkbage left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do you think we didn't catch this in the code or the simple tests we added?

Have we hit terminal complexity?

@sizebot
Copy link

sizebot commented Nov 8, 2018

Details of bundled changes.

Comparing: 3d8bda7...c41126f

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
Copy link
Collaborator Author

acdlite commented Nov 8, 2018

@sebmarkbage I blame it on sloppiness on my part when I was updating the tests in #13823. But yeah, it's getting pretty complex, too.

There are a few things in our Suspense implementation that are unusual:

  • Custom reconciliation. We conditionally wrap the children in a fragment while preserving their state.
  • Outside concurrent mode, we commit the suspended tree in an incomplete state. By itself, this is similar to our hidden trick.
  • Semantic divergence between concurrent and synchronous mode. Usually the only difference between the two modes is scheduling. There are some second order things that diverge, but synchronous mode is essentially a subset of the semantics of concurrent mode. But with Suspense, the semantics are forked.
  • Hiding a tree instead of deleting it. Conceptually similar to the hidden trick except we have to do the hiding ourselves, by toggling the nearest host children.

Any one of these quirks by themselves isn't so complex, but we have to consider all of them together.

I still think they're worth it, but if we ever decide it's too much, we might need to drop one of these constraints.

Vestigial behavior that should have been removed in facebook#13823.

Found using the Suspense fuzz tester in facebook#14147.
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.

4 participants