[Synchronous Suspense] Reuse deletions from primary tree #14133
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
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 |
gaearon
reviewed
Nov 7, 2018
| @@ -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.
So this was the thing that caused us to "forget" about deletions?
gaearon
reviewed
Nov 7, 2018
| // 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.
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.
gaearon
reviewed
Nov 7, 2018
| // 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.
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.
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.)
gaearon
approved these changes
Nov 7, 2018
|
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.
acdlite
force-pushed
the
reuse-deletions
branch
from
63ebb07
to
7788bee
Compare
|
This makes sense to me but is also scary. :/ |
acdlite
added a commit
to acdlite/react
that referenced
this pull request
Nov 8, 2018
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.
acdlite
added a commit
to acdlite/react
that referenced
this pull request
Nov 8, 2018
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.
Merged
acdlite
added a commit
to acdlite/react
that referenced
this pull request
Nov 8, 2018
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.
acdlite
added a commit
to acdlite/react
that referenced
this pull request
Nov 8, 2018
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.
acdlite
added a commit
to acdlite/react
that referenced
this pull request
Nov 8, 2018
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.
acdlite
added a commit
to acdlite/react
that referenced
this pull request
Nov 8, 2018
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.
acdlite
added a commit
to acdlite/react
that referenced
this pull request
Nov 8, 2018
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.
acdlite
added a commit
that referenced
this pull request
Nov 9, 2018
* 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.
Merged
This was referenced Dec 1, 2018
jetoneza
pushed a commit
to jetoneza/react
that referenced
this pull request
Jan 23, 2019
) 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.
jetoneza
pushed a commit
to jetoneza/react
that referenced
this pull request
Jan 23, 2019
* 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.
n8schloss
pushed a commit
to n8schloss/react
that referenced
this pull request
Jan 31, 2019
) 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.
n8schloss
pushed a commit
to n8schloss/react
that referenced
this pull request
Jan 31, 2019
* 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.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.