-
Notifications
You must be signed in to change notification settings - Fork 46.9k
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] Reuse deletions from primary tree #14133
Conversation
ReactDOM: size: -0.4%, gzip: -0.3% Details of bundled changes.Comparing: aa1ffe4...7788bee react-dom
react-art
react-test-renderer
react-reconciler
react-native-renderer
scheduler
Generated by 🚫 dangerJS |
@@ -993,7 +993,6 @@ function completeUnitOfWork(workInProgress: Fiber): Fiber | null { | |||
|
|||
if (nextUnitOfWork !== null) { | |||
// Completing this fiber spawned new work. Work on that next. | |||
nextUnitOfWork.firstEffect = nextUnitOfWork.lastEffect = null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So this was the thing that caused us to "forget" about deletions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes
// We just switched from the fallback to the normal children. Delete | ||
// the fallback. | ||
// TODO: Would it be better to store the fallback fragment on | ||
// the stateNode during the begin phase? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why would it be better?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To avoid the confusing series of checks this comment is wrapped in. Could be replaced by checking if the stateNode is null.
I'm not sure that's actually better though. Kinda gross either way.
// the stateNode during the begin phase? | ||
const currentFallbackChild: Fiber | null = (current.child: any).sibling; | ||
if (currentFallbackChild !== null) { | ||
reconcileChildFibers( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be simpler if instead of deleting the fallback we kept it hidden? Although that seems bad from memory perspective.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be simpler because we would always render two fragments, instead of switching between fragments and no fragments like we do now. That's why updateSuspenseComponent
has so much code in it; we don't do this type of custom reconciliation anywhere else, yet. Combined with the other weird things we do with Suspense (committing incomplete trees in sync mode, hiding instead of deleting after a timeout) it's gotten pretty complex. Trade off still seems worth it, but unfortunately that means it might take longer to flush out all these edge cases.
(I keep trying to think of a good fuzz test, but I can't. Haven't given up though.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I understand this PR correctly it just lets the suspense follow the normal flow of how effects are marked instead of trying to clear/reuse them separately, and that lets us not lose deletions. I think this makes sense to me.
Going to merge so we can try this out on www. @sebmarkbage let me know if this fix doesn't match what we verbally agreed to yesterday. |
Fixes a bug where deletion effects in the primary tree were dropped before entering the second render pass. Because we no longer reset the effect list after the first render pass, I've also moved the deletion of the fallback children to the complete phase, after the tree successfully renders without suspending. Will need to revisit this heuristic when we implement resuming.
63ebb07
to
7788bee
Compare
This makes sense to me but is also scary. :/ |
The fuzzer works by generating a random tree of React elements. The tree two types of custom components: - A Text component suspends rendering on initial mount for a fuzzy duration of time. It may update a fuzzy number of times; each update supsends for a fuzzy duration of time. - A Container component wraps some children. It may remount its children a fuzzy number of times, by updating its key. The tree may also include nested Suspense components. After this tree is generated, the tester sets a flag to temporarily disable Text components from suspending. The tree is rendered synchronously. The output of this render is the expected output. Then the tester flips the flag back to enable suspending. It renders the tree again. This time the Text components will suspend for the amount of time configured by the props. The tester waits until everything has resolved. The resolved output is then compared to the expected output generated in the previous step. Finally, we render once more, but this time in concurrent mode. Once again, the resolved output is generated to the expected output. I tested by commenting out various parts of the Suspense implementation to see if broke in the expected way. I also confirmed that it would have caught facebook#14133, a recent bug related to deletions.
The fuzzer works by generating a random tree of React elements. The tree two types of custom components: - A Text component suspends rendering on initial mount for a fuzzy duration of time. It may update a fuzzy number of times; each update supsends for a fuzzy duration of time. - A Container component wraps some children. It may remount its children a fuzzy number of times, by updating its key. The tree may also include nested Suspense components. After this tree is generated, the tester sets a flag to temporarily disable Text components from suspending. The tree is rendered synchronously. The output of this render is the expected output. Then the tester flips the flag back to enable suspending. It renders the tree again. This time the Text components will suspend for the amount of time configured by the props. The tester waits until everything has resolved. The resolved output is then compared to the expected output generated in the previous step. Finally, we render once more, but this time in concurrent mode. Once again, the resolved output is generated to the expected output. I tested by commenting out various parts of the Suspense implementation to see if broke in the expected way. I also confirmed that it would have caught facebook#14133, a recent bug related to deletions.
The fuzzer works by generating a random tree of React elements. The tree two types of custom components: - A Text component suspends rendering on initial mount for a fuzzy duration of time. It may update a fuzzy number of times; each update supsends for a fuzzy duration of time. - A Container component wraps some children. It may remount its children a fuzzy number of times, by updating its key. The tree may also include nested Suspense components. After this tree is generated, the tester sets a flag to temporarily disable Text components from suspending. The tree is rendered synchronously. The output of this render is the expected output. Then the tester flips the flag back to enable suspending. It renders the tree again. This time the Text components will suspend for the amount of time configured by the props. The tester waits until everything has resolved. The resolved output is then compared to the expected output generated in the previous step. Finally, we render once more, but this time in concurrent mode. Once again, the resolved output is compared to the expected output. I tested by commenting out various parts of the Suspense implementation to see if broke in the expected way. I also confirmed that it would have caught facebook#14133, a recent bug related to deletions.
The fuzzer works by generating a random tree of React elements. The tree two types of custom components: - A Text component suspends rendering on initial mount for a fuzzy duration of time. It may update a fuzzy number of times; each update supsends for a fuzzy duration of time. - A Container component wraps some children. It may remount its children a fuzzy number of times, by updating its key. The tree may also include nested Suspense components. After this tree is generated, the tester sets a flag to temporarily disable Text components from suspending. The tree is rendered synchronously. The output of this render is the expected output. Then the tester flips the flag back to enable suspending. It renders the tree again. This time the Text components will suspend for the amount of time configured by the props. The tester waits until everything has resolved. The resolved output is then compared to the expected output generated in the previous step. Finally, we render once more, but this time in concurrent mode. Once again, the resolved output is compared to the expected output. I tested by commenting out various parts of the Suspense implementation to see if broke in the expected way. I also confirmed that it would have caught facebook#14133, a recent bug related to deletions.
The fuzzer works by generating a random tree of React elements. The tree two types of custom components: - A Text component suspends rendering on initial mount for a fuzzy duration of time. It may update a fuzzy number of times; each update supsends for a fuzzy duration of time. - A Container component wraps some children. It may remount its children a fuzzy number of times, by updating its key. The tree may also include nested Suspense components. After this tree is generated, the tester sets a flag to temporarily disable Text components from suspending. The tree is rendered synchronously. The output of this render is the expected output. Then the tester flips the flag back to enable suspending. It renders the tree again. This time the Text components will suspend for the amount of time configured by the props. The tester waits until everything has resolved. The resolved output is then compared to the expected output generated in the previous step. Finally, we render once more, but this time in concurrent mode. Once again, the resolved output is compared to the expected output. I tested by commenting out various parts of the Suspense implementation to see if broke in the expected way. I also confirmed that it would have caught facebook#14133, a recent bug related to deletions.
The fuzzer works by generating a random tree of React elements. The tree two types of custom components: - A Text component suspends rendering on initial mount for a fuzzy duration of time. It may update a fuzzy number of times; each update supsends for a fuzzy duration of time. - A Container component wraps some children. It may remount its children a fuzzy number of times, by updating its key. The tree may also include nested Suspense components. After this tree is generated, the tester sets a flag to temporarily disable Text components from suspending. The tree is rendered synchronously. The output of this render is the expected output. Then the tester flips the flag back to enable suspending. It renders the tree again. This time the Text components will suspend for the amount of time configured by the props. The tester waits until everything has resolved. The resolved output is then compared to the expected output generated in the previous step. Finally, we render once more, but this time in concurrent mode. Once again, the resolved output is compared to the expected output. I tested by commenting out various parts of the Suspense implementation to see if broke in the expected way. I also confirmed that it would have caught facebook#14133, a recent bug related to deletions.
The fuzzer works by generating a random tree of React elements. The tree two types of custom components: - A Text component suspends rendering on initial mount for a fuzzy duration of time. It may update a fuzzy number of times; each update supsends for a fuzzy duration of time. - A Container component wraps some children. It may remount its children a fuzzy number of times, by updating its key. The tree may also include nested Suspense components. After this tree is generated, the tester sets a flag to temporarily disable Text components from suspending. The tree is rendered synchronously. The output of this render is the expected output. Then the tester flips the flag back to enable suspending. It renders the tree again. This time the Text components will suspend for the amount of time configured by the props. The tester waits until everything has resolved. The resolved output is then compared to the expected output generated in the previous step. Finally, we render once more, but this time in concurrent mode. Once again, the resolved output is compared to the expected output. I tested by commenting out various parts of the Suspense implementation to see if broke in the expected way. I also confirmed that it would have caught facebook#14133, a recent bug related to deletions.
* Don't warn if an unmounted component is pinged * Suspense fuzz tester The fuzzer works by generating a random tree of React elements. The tree two types of custom components: - A Text component suspends rendering on initial mount for a fuzzy duration of time. It may update a fuzzy number of times; each update supsends for a fuzzy duration of time. - A Container component wraps some children. It may remount its children a fuzzy number of times, by updating its key. The tree may also include nested Suspense components. After this tree is generated, the tester sets a flag to temporarily disable Text components from suspending. The tree is rendered synchronously. The output of this render is the expected output. Then the tester flips the flag back to enable suspending. It renders the tree again. This time the Text components will suspend for the amount of time configured by the props. The tester waits until everything has resolved. The resolved output is then compared to the expected output generated in the previous step. Finally, we render once more, but this time in concurrent mode. Once again, the resolved output is compared to the expected output. I tested by commenting out various parts of the Suspense implementation to see if broke in the expected way. I also confirmed that it would have caught #14133, a recent bug related to deletions. * When a generated test case fails, log its input * Moar fuzziness Adds more fuzziness to the generated tests. Specifcally, introduces nested Suspense cases, where the fallback of a Suspense component also suspends. This flushed out a bug (yay!) whose test case I've hard coded. * Use seeded random number generator So if there's a failure, we can bisect.
) Fixes a bug where deletion effects in the primary tree were dropped before entering the second render pass. Because we no longer reset the effect list after the first render pass, I've also moved the deletion of the fallback children to the complete phase, after the tree successfully renders without suspending. Will need to revisit this heuristic when we implement resuming.
* Don't warn if an unmounted component is pinged * Suspense fuzz tester The fuzzer works by generating a random tree of React elements. The tree two types of custom components: - A Text component suspends rendering on initial mount for a fuzzy duration of time. It may update a fuzzy number of times; each update supsends for a fuzzy duration of time. - A Container component wraps some children. It may remount its children a fuzzy number of times, by updating its key. The tree may also include nested Suspense components. After this tree is generated, the tester sets a flag to temporarily disable Text components from suspending. The tree is rendered synchronously. The output of this render is the expected output. Then the tester flips the flag back to enable suspending. It renders the tree again. This time the Text components will suspend for the amount of time configured by the props. The tester waits until everything has resolved. The resolved output is then compared to the expected output generated in the previous step. Finally, we render once more, but this time in concurrent mode. Once again, the resolved output is compared to the expected output. I tested by commenting out various parts of the Suspense implementation to see if broke in the expected way. I also confirmed that it would have caught facebook#14133, a recent bug related to deletions. * When a generated test case fails, log its input * Moar fuzziness Adds more fuzziness to the generated tests. Specifcally, introduces nested Suspense cases, where the fallback of a Suspense component also suspends. This flushed out a bug (yay!) whose test case I've hard coded. * Use seeded random number generator So if there's a failure, we can bisect.
) Fixes a bug where deletion effects in the primary tree were dropped before entering the second render pass. Because we no longer reset the effect list after the first render pass, I've also moved the deletion of the fallback children to the complete phase, after the tree successfully renders without suspending. Will need to revisit this heuristic when we implement resuming.
* Don't warn if an unmounted component is pinged * Suspense fuzz tester The fuzzer works by generating a random tree of React elements. The tree two types of custom components: - A Text component suspends rendering on initial mount for a fuzzy duration of time. It may update a fuzzy number of times; each update supsends for a fuzzy duration of time. - A Container component wraps some children. It may remount its children a fuzzy number of times, by updating its key. The tree may also include nested Suspense components. After this tree is generated, the tester sets a flag to temporarily disable Text components from suspending. The tree is rendered synchronously. The output of this render is the expected output. Then the tester flips the flag back to enable suspending. It renders the tree again. This time the Text components will suspend for the amount of time configured by the props. The tester waits until everything has resolved. The resolved output is then compared to the expected output generated in the previous step. Finally, we render once more, but this time in concurrent mode. Once again, the resolved output is compared to the expected output. I tested by commenting out various parts of the Suspense implementation to see if broke in the expected way. I also confirmed that it would have caught facebook#14133, a recent bug related to deletions. * When a generated test case fails, log its input * Moar fuzziness Adds more fuzziness to the generated tests. Specifcally, introduces nested Suspense cases, where the fallback of a Suspense component also suspends. This flushed out a bug (yay!) whose test case I've hard coded. * Use seeded random number generator So if there's a failure, we can bisect.
Fixes a bug where deletion effects in the primary tree were dropped before entering the second render pass.
Because we no longer reset the effect list after the first render pass, I've also moved the deletion of the fallback children to the complete phase, after the tree successfully renders without suspending.
Will need to revisit this heuristic when we implement resuming.