Skip to content

[Fiber] Add more tests and fix an issue with incremental error handling #8212

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.

Sign up for GitHub

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

Closed
wants to merge 3 commits into from
Closed

[Fiber] Add more tests and fix an issue with incremental error handling #8212

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
acb896d
Add more tests for incremental error handling
gaearon Nov 5, 2016
12c7d2d
Always reuse progressChild when starting Task work
gaearon Nov 5, 2016
00ab372
Verify batched updates get scheduled despite errors
gaearon Nov 5, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 13 additions & 2 deletions scripts/fiber/tests-passing.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -793,8 +793,19 @@ src/renderers/shared/fiber/__tests__/ReactIncremental-test.js
* can nest batchedUpdates

src/renderers/shared/fiber/__tests__/ReactIncrementalErrorHandling-test.js
* catches render error in a boundary during mounting
* propagates an error from a noop error boundary
* catches render error in a boundary during full deferred mounting
* catches render error in a boundary during partial deferred mounting
* catches render error in a boundary during animation mounting
* catches render error in a boundary during synchronous mounting
* catches render error in a boundary during batched mounting
* propagates an error from a noop error boundary during full deferred mounting
* propagates an error from a noop error boundary during partial deferred mounting
* propagates an error from a noop error boundary during animation mounting
* propagates an error from a noop error boundary during synchronous mounting
* propagates an error from a noop error boundary during batched mounting
* applies batched updates regardless despite errors in scheduling
* applies nested batched updates despite errors in scheduling
* applies sync updates regardless despite errors in scheduling
* can schedule updates after uncaught error in render on mount
* can schedule updates after uncaught error in render on update
* can schedule updates after uncaught error during umounting
Expand Down
6 changes: 5 additions & 1 deletion src/renderers/shared/fiber/ReactFiberBeginWork.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ var {
var {
NoWork,
OffscreenPriority,
TaskPriority,
} = require('ReactPriorityLevel');
var {
Placement,
Expand Down Expand Up @@ -365,9 +366,12 @@ module.exports = function<T, P, I, TI, C>(
workInProgress.firstEffect = null;
workInProgress.lastEffect = null;

if (workInProgress.progressedPriority === priorityLevel) {
if (workInProgress.progressedPriority === priorityLevel ||
priorityLevel === TaskPriority) {
Copy link
Collaborator

@sebmarkbage sebmarkbage Nov 8, 2016

Choose a reason for hiding this comment

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

I don't think that this is correct. Let's say we had a very low-priority render that had started deleting one child but it never completed. Instead it did some other higher priority updates to it.

Then suddenly something in a boundary fails (or get another Task priority update). Then the child would be remounted instead of reused.

I don't have a better solution atm but I'll think about it some more. I don't fully understand how the failure case happens now.

Would always force unmounting children in an error boundary help with this case?

Copy link
Collaborator

@acdlite acdlite Nov 8, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

Would always force unmounting children in an error boundary help with this case?

It would because we'd be recreating the whole tree instead of trying to reuse failed work... I think?

Copy link
Collaborator Author

@gaearon gaearon Nov 8, 2016

Choose a reason for hiding this comment

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

Would always force unmounting children in an error boundary help with this case?

I'm not sure. Maybe let's come back to this after #8227?

// If we have progressed work on this priority level already, we can
// proceed this that as the child.
// We also can reuse the child if we're doing Task work. This avoids
// having the error boundaries doing the failed work twice before mount.
workInProgress.child = workInProgress.progressedChild;
}

Expand Down
Loading