Skip to content

[Fiber] New error boundary semantics #8304

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

Merged
merged 19 commits into from
Dec 1, 2016
Merged

[Fiber] New error boundary semantics #8304

Changes from 1 commit
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
7967679
New error boundary semantics
acdlite Nov 16, 2016
88be836
Fix broken setState callback test
acdlite Nov 16, 2016
69796b3
Exit early in scheduleUpdate if a node's priority matches
acdlite Nov 12, 2016
614cd4b
Add test for "unwinding" context when an error interrupts rendering
acdlite Nov 17, 2016
4f126f5
Switch over primary effect types only
acdlite Nov 17, 2016
a61de42
Only continue the work loop if the error was successfully captured
acdlite Nov 17, 2016
fad5cb5
Add try/catch/finally blocks around commit phase passes
acdlite Nov 28, 2016
ece9f7b
NoopBoundary -> RethrowBoundary
acdlite Nov 28, 2016
226ee8f
Only throw uncaught error once there is no more work to perform
acdlite Nov 29, 2016
7386e82
Add more tests for incremental error handling
gaearon Nov 5, 2016
d27cada
Verify batched updates get scheduled despite errors
gaearon Nov 5, 2016
cc60f25
Remove outdated comment
gaearon Nov 30, 2016
778183e
Record tests
gaearon Nov 30, 2016
54978e9
Always reset nextUnitOfWork on error
gaearon Nov 30, 2016
c1f2eb2
Add a passing test for unmounting behavior on crashed tree
gaearon Nov 30, 2016
46581a3
Top-level errors
acdlite Nov 30, 2016
b32c86e
Remove outdated comment
gaearon Nov 30, 2016
7bc10c0
Separate Rethrow and Noop scenarios in boundary tests
gaearon Nov 30, 2016
cf20dfa
Move try block outside the commit loops
acdlite Nov 30, 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
Prev Previous commit
Move try block outside the commit loops
  • Loading branch information
@acdlite
acdlite committed Nov 30, 2016
commit cf20dfa99841fcaee0baaf4ae30a550b0e70df5b
135 changes: 74 additions & 61 deletions src/renderers/shared/fiber/ReactFiberScheduler.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -165,48 +165,54 @@ module.exports = function<T, P, I, TI, C>(config : HostConfig<T, P, I, TI, C>) {
// First, we'll perform all the host insertions, updates, deletions and
// ref unmounts.
let effectfulFiber = finishedWork.firstEffect;
while (effectfulFiber) {
pass1: while (true) {
try {
Copy link
Collaborator

@sebmarkbage sebmarkbage Nov 30, 2016

Choose a reason for hiding this comment

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

These try blocks should be outside the while loop. That way we can hoist them out of this function and avoid going in and out of an deopt path for each iteration.

Copy link
Collaborator Author

@acdlite acdlite Nov 30, 2016

Choose a reason for hiding this comment

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

Is my latest commit okay? Had to add another loop outside so that we can continue after an error

// The following switch statement is only concerned about placement,
// updates, and deletions. To avoid needing to add a case for every
// possible bitmap value, we remove the secondary effects from the
// effect tag and switch on that value.
let primaryEffectTag = effectfulFiber.effectTag & ~(Callback | Err);
switch (primaryEffectTag) {
case Placement: {
commitPlacement(effectfulFiber);
// Clear the "placement" from effect tag so that we know that this is inserted, before
// any life-cycles like componentDidMount gets called.
effectfulFiber.effectTag &= ~Placement;
break;
}
case PlacementAndUpdate: {
// Placement
commitPlacement(effectfulFiber);
// Clear the "placement" from effect tag so that we know that this is inserted, before
// any life-cycles like componentDidMount gets called.
effectfulFiber.effectTag &= ~Placement;

// Update
const current = effectfulFiber.alternate;
commitWork(current, effectfulFiber);
break;
}
case Update: {
const current = effectfulFiber.alternate;
commitWork(current, effectfulFiber);
break;
}
case Deletion: {
commitDeletion(effectfulFiber);
break;
while (effectfulFiber) {
// The following switch statement is only concerned about placement,
// updates, and deletions. To avoid needing to add a case for every
// possible bitmap value, we remove the secondary effects from the
// effect tag and switch on that value.
let primaryEffectTag = effectfulFiber.effectTag & ~(Callback | Err);
switch (primaryEffectTag) {
case Placement: {
commitPlacement(effectfulFiber);
// Clear the "placement" from effect tag so that we know that this is inserted, before
// any life-cycles like componentDidMount gets called.
effectfulFiber.effectTag &= ~Placement;
break;
}
case PlacementAndUpdate: {
// Placement
commitPlacement(effectfulFiber);
// Clear the "placement" from effect tag so that we know that this is inserted, before
// any life-cycles like componentDidMount gets called.
effectfulFiber.effectTag &= ~Placement;

// Update
const current = effectfulFiber.alternate;
commitWork(current, effectfulFiber);
break;
}
case Update: {
const current = effectfulFiber.alternate;
commitWork(current, effectfulFiber);
break;
}
case Deletion: {
commitDeletion(effectfulFiber);
break;
}
}
effectfulFiber = effectfulFiber.nextEffect;
}
} catch (error) {
captureError(effectfulFiber, error, false);
} finally {
effectfulFiber = effectfulFiber.nextEffect;
if (effectfulFiber) {
captureError(effectfulFiber, error, false);
effectfulFiber = effectfulFiber.nextEffect;
continue pass1;
}
}
break;
}

// Finally if the root itself had an effect, we perform that since it is
Expand All @@ -222,35 +228,42 @@ module.exports = function<T, P, I, TI, C>(config : HostConfig<T, P, I, TI, C>) {
// happens as a separate pass so that all effects in the entire tree have
// already been invoked.
effectfulFiber = finishedWork.firstEffect;
while (effectfulFiber) {
const previousPriorityContext = priorityContext;
priorityContext = TaskPriority;
const previousPriorityContext = priorityContext;
priorityContext = TaskPriority;
pass2: while (true) {
Copy link
Collaborator

@gaearon gaearon Nov 18, 2016

Choose a reason for hiding this comment

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

Removing this try/finally block seems responsible for some of the test failures.
If I add it back I get:

--- a/scripts/fiber/tests-passing.txt
+++ b/scripts/fiber/tests-passing.txt
@@ -470,7 +470,6 @@ src/renderers/dom/__tests__/ReactDOMProduction-test.js
 * should use prod React
 * should handle a simple flow
 * should call lifecycle methods
-* should throw with an error code in production

 src/renderers/dom/fiber/__tests__/ReactDOMFiber-test.js
 * should render strings as children
@@ -1021,8 +1020,12 @@ src/renderers/shared/shared/event/eventPlugins/__tests__/ResponderEventPlugin-te

 src/renderers/shared/stack/reconciler/__tests__/ReactComponent-test.js
 * should throw when supplying a ref outside of render method
-* should support refs on owned components
+* should warn when children are mutated during update
 * should not have refs on unmounted components
+* should support new-style refs
+* should support new-style refs with mixed-up owners
+* should call refs at the correct time
+* fires the callback after a component is rendered

 src/renderers/shared/stack/reconciler/__tests__/ReactComponentLifeCycle-test.js
 * should not reuse an instance when it has been unmounted
@@ -1185,8 +1188,9 @@ src/renderers/shared/stack/reconciler/__tests__/ReactStatelessComponent-test.js
 * should update stateless component
 * should unmount stateless component
 * should pass context thru stateless component
-* should provide a null ref
+* should use correct name in key warning
 * should support default props and prop types
+* should receive context
 * should work with arrow functions
 * should allow simple functions to return null
 * should allow simple functions to return false

try {
const current = effectfulFiber.alternate;
// Use Task priority for lifecycle updates
if (effectfulFiber.effectTag & (Update | Callback)) {
commitLifeCycles(current, effectfulFiber);
}
while (effectfulFiber) {
const current = effectfulFiber.alternate;
// Use Task priority for lifecycle updates
if (effectfulFiber.effectTag & (Update | Callback)) {
commitLifeCycles(current, effectfulFiber);
}

if (effectfulFiber.effectTag & Err) {
commitErrorHandling(effectfulFiber);
if (effectfulFiber.effectTag & Err) {
commitErrorHandling(effectfulFiber);
}

const next = effectfulFiber.nextEffect;
// Ensure that we clean these up so that we don't accidentally keep them.
// I'm not actually sure this matters because we can't reset firstEffect
// and lastEffect since they're on every node, not just the effectful
// ones. So we have to clean everything as we reuse nodes anyway.
effectfulFiber.nextEffect = null;
// Ensure that we reset the effectTag here so that we can rely on effect
// tags to reason about the current life-cycle.
effectfulFiber = next;
}
} catch (error) {
captureError(effectfulFiber, error, false);
} finally {
// Clean-up
priorityContext = previousPriorityContext;

const next = effectfulFiber.nextEffect;
// Ensure that we clean these up so that we don't accidentally keep them.
// I'm not actually sure this matters because we can't reset firstEffect
// and lastEffect since they're on every node, not just the effectful
// ones. So we have to clean everything as we reuse nodes anyway.
effectfulFiber.nextEffect = null;
// Ensure that we reset the effectTag here so that we can rely on effect
// tags to reason about the current life-cycle.
effectfulFiber = next;
if (effectfulFiber) {
captureError(effectfulFiber, error, false);
const next = effectfulFiber.nextEffect;
effectfulFiber.nextEffect = null;
effectfulFiber = next;
continue pass2;
}
}
priorityContext = previousPriorityContext;
break;
}

// Lifecycles on the root itself
Expand Down