Bugfix: nextFlushedRoot should always be set when performing work#11558
Merged
acdlite merged 1 commit intofacebook:masterfrom Nov 15, 2017
Merged
Bugfix: nextFlushedRoot should always be set when performing work#11558acdlite merged 1 commit intofacebook:masterfrom
acdlite merged 1 commit intofacebook:masterfrom
Conversation
acdlite
force-pushed
the
sync-initial-mount-error
branch
from
fc52d45 to
0486807
Compare
Collaborator
|
I don't think I know how to review this but thanks for fixing! (Is there a way we could have caught this with a fuzz test? ;)) |
Collaborator
Author
@sophiebits Theoretically yes. Hard to think of a good error handling fuzz test, though. I'll try to come up with one before the next error handling refactor. |
clemmy
reviewed
Nov 15, 2017
| expect(caughtError.message).toBe('child sad'); | ||
| }); | ||
|
|
||
| it('uncaught error inside unbatched initial mount', () => { |
Contributor
There was a problem hiding this comment.
Test name is a little bit weird. Maybe propagates error instead?
Fixes an issue where performWorkOnRoot was called, but nextFlushedRoot was not set. This happened in a special branch where we synchronously flush newly mounted DOM trees outside the normal work loop. Arguably, performWorkOnRoot should read from the globally assigned root and expiration time instead of accepting arguments, since those arguments are expected to be the same as the global values, anyway. I decided against that since the global values could be null, so reading from them would require extra null checks.
acdlite
force-pushed
the
sync-initial-mount-error
branch
from
0486807 to
7bbe186
Compare
clemmy
approved these changes
Nov 15, 2017
Ethan-Arrowood
pushed a commit
to Ethan-Arrowood/react
that referenced
this pull request
Dec 8, 2017
…cebook#11558) Fixes an issue where performWorkOnRoot was called, but nextFlushedRoot was not set. This happened in a special branch where we synchronously flush newly mounted DOM trees outside the normal work loop. Arguably, performWorkOnRoot should read from the globally assigned root and expiration time instead of accepting arguments, since those arguments are expected to be the same as the global values, anyway. I decided against that since the global values could be null, so reading from them would require extra null checks.
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
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 an issue where
performWorkOnRootwas called, butnextFlushedRootwas not set. This happened in a special branch where we synchronously flush newly mounted DOM trees outside the normal work loop.Arguably,
performWorkOnRootshould read from the globally assigned root and expiration time instead of accepting arguments, since those arguments are expected to be the same as the global values, anyway. I decided against that since the global values could be null, so reading from them would require extra null checks.