[Fiber] Force rerender a failed tree #8227

acdlite · 2016-11-07T22:58:44Z

This lets us preserve the invariant that if a tree contains pending work, its root is scheduled. Previously, a tree with an uncaught error broke this assumption.

Now we can exit early in scheduleUpdate once we reach a node whose priority matches.

Closes #8222

sebmarkbage · 2016-11-07T23:12:55Z

Curious to hear what @gaearon thinks about this. I think it is possible to unmount it from React's perspective but avoid removing the child at the root if we want to leave it in.

gaearon · 2016-11-08T16:10:11Z

I think the least surprising behavior is:

All unmount lifecycles fire (safely, i.e. errors in them are ignored)
isMounted() for components returns false and setState() is a no-op
All event handlers are removed, but the DOM tree stays at the same spot, it's just "non-interactive" now
Next unmountComponentAtNode() removes the DOM tree
Next render() also removes the DOM tree, and mounts a new one

gaearon

Let's keep the DOM.

acdlite · 2016-11-08T18:08:36Z

I assume we should do the same thing for errors caught by boundaries? "Unmount" the tree, without actually removing the DOM nodes, and mount a new one the next time the boundary is re-rendered.

gaearon · 2016-11-08T18:31:22Z

Would the difference be observable if boundaries are processed synchronously?

acdlite · 2016-11-08T18:34:39Z

Yes because the entire failed tree below the boundary is unmounted regardless, whereas in the current implementation parts of the failed tree may be reused.

sebmarkbage · 2016-11-08T18:48:52Z

I just want to frame the discussion a bit:

The worst case scenario isn't that the user get frustrated that the UI is non-interactive or confusing. That's the best case. We have to weight that against the worst case.

The worst case, would likely be that you get into an inconsistent state so that internal values are not represented on the screen and then an action gets taken based on that. For example, a user spends the wrong amount, pays for the wrong thing, or worse, "Like" your ex's photo instead of your current significant other.

I think it is very easy to get in to an inconsistent state for a subtree that has errored. Given that life-cycles can mutate instances etc. Recovering from that may work in 99% of cases but in the 1% it doesn't, it can be devastating.

So, my preference would be to force an unmount even in the error boundary case. So that the view gets a forced rerender.

However, in the forced rerender case, when do you remove the child node? You don't want to remove it in case the error boundary then fails, because then it is gone. I guess you don't, and just let the regular reconciliation remove it if it succeeds.

acdlite · 2016-11-08T18:54:29Z

However, in the forced rerender case, when do you remove the child node? You don't want to remove it in case the error boundary then fails, because then it is gone. I guess you don't, and just let the regular reconciliation remove it if it succeeds.

That makes sense to me.

I'll give this a shot.

Outdated

gaearon · 2016-11-08T19:08:19Z

So, my preference would be to force an unmount even in the error boundary case. So that the view gets a forced rerender.

👍

acdlite · 2016-11-11T01:27:58Z

@gaearon Ready for review!

Now that boundary's are force remounted, I had to tweak some of the unit tests in ReactErrorBoundaries-test.js. I believe the changes I made correctly match the new semantics but they might deserve some extra scrutiny.

gaearon

I think we need to fix duplicate componentWillUnmount calls.

gaearon · 2016-11-11T15:07:32Z

src/renderers/shared/stack/reconciler/__tests__/ReactErrorBoundaries-test.js

      ...(ReactDOMFeatureFlags.useFiber ? [
+        // Fiber unmounts an error boundary's children
+        'Normal componentWillUnmount',


There is another Normal componentWillUnmount below right before the array ends.
Does this means we now call it twice? Doesn't seem right.

gaearon · 2016-11-11T15:08:00Z

src/renderers/shared/stack/reconciler/__tests__/ReactErrorBoundaries-test.js

      ...(ReactDOMFeatureFlags.useFiber ? [
+        // Fiber unmounts an error boundary's children
+        'Normal componentWillUnmount',


Same here. There's another Normal componentWillUnmount later.

gaearon · 2016-11-11T15:08:39Z

src/renderers/shared/stack/reconciler/__tests__/ReactErrorBoundaries-test.js

      ...(ReactDOMFeatureFlags.useFiber ? [
+        // Fiber unmounts an error boundary's children
+        'Normal componentWillUnmount',
+        'BrokenComponentWillReceiveProps componentWillUnmount',


This one is also duplicated: see BrokenComponentWillReceiveProps componentWillUnmount later in the array.

gaearon · 2016-11-11T15:13:48Z

src/renderers/shared/fiber/ReactFiberCommitWork.js

@@ -193,11 +195,14 @@ module.exports = function<T, P, I, TI, C>(
        commitNestedUnmounts(node);


Does this call (or any commitUnmount() calls it makes) also need to be guarded by !ignoreTop?
I don't fully understand what's happening here but it offers another path to commitUnmount().

gaearon · 2016-11-11T15:17:58Z

src/renderers/shared/fiber/ReactFiberCommitWork.js

@@ -193,11 +195,14 @@ module.exports = function<T, P, I, TI, C>(
        commitNestedUnmounts(node);
        // After all the children have unmounted, it is now safe to remove the
        // node from the tree.
-        if (parent) {
+        if (parent && !ignoreHostNode) {
          removeChild(parent, node.stateNode);


Please bear with me as I don't fully understand this method yet.
Can removeChild() below run more than once?
If it can, ignoreHostNode should be named ignoreHostNodes?

gaearon · 2016-11-11T15:20:08Z

src/renderers/shared/fiber/ReactFiberCommitWork.js

          removeChild(parent, node.stateNode);
        }
      } else {
-        commitUnmount(node);
+        // If ignoreTop is true, don't unmount the top fiber
+        if (!ignoreTop || node !== current) {


Similar question here. Why are we sure this is a "top" node? It isn't quite clear to me.
Is there only one "top" node, or can this block run more than twice in a loop?
Is there a better name for ignoreTop? I don't understand which fiber exactly would be called a "top" node.

Maybe it would be easier to understand if the comment emphasized when this happens (e.g. "If node and current are different, it means X. When ignoreTop is set, it means Y. Therefore we do Z to implement this [scenario described from user's point of view].")

gaearon · 2016-11-11T15:23:43Z

src/renderers/shared/fiber/ReactFiberCommitWork.js

+    // the tree errored, we won't actually remove the host nodes; we'll leave
+    // them there until the tree is rerendered.
+    unmountHostComponents(null, current, true, false);
+    current.child = null;


Do we need to clear progressedChild?

gaearon · 2016-11-11T15:23:54Z

src/renderers/shared/fiber/ReactFiberCommitWork.js

+      unmountHostComponents(parent, current, true, true);
+    }
+
+    boundary.child = null;


Same question about progressedChild here.

gaearon · 2016-11-11T15:27:57Z

src/renderers/shared/fiber/ReactFiberScheduler.js

+            root = nextScheduledRoot;
+            while (root) {
+              forceUnmountFailedRoot(root);
+              root.current.pendingWorkPriority = NoWork;


Do we need to update root.current.alternate here?

gaearon · 2016-11-11T15:31:20Z

src/renderers/shared/fiber/ReactFiberScheduler.js

@@ -753,27 +753,31 @@ module.exports = function<T, P, I, TI, C>(config : HostConfig<T, P, I, TI, C>) {
      priorityLevel = TaskPriority;
    }


What does this commit fix? Is there a test that was failing before? Is this a performance optimization?

Perf optimization, unobservable

Can you add a comment where you set shouldContinue to false explaining in which circumstances it will get set to true?

sebmarkbage · 2016-11-11T16:27:05Z

src/renderers/shared/fiber/ReactFiberCommitWork.js

@@ -184,7 +185,7 @@ module.exports = function<T, P, I, TI, C>(
    }
  }

-  function unmountHostComponents(parent, current): void {
+  function unmountHostComponents(parent, current, ignoreHostNode, ignoreTop): void {


I think we can find a way to not use these flags.

A normal unmount won't schedule a removeChild anywhere but the top root where it was deleted. All we have to do is special case the root and then the rest of the traversal can be exactly the same.

Similarly, ignoreTop wouldn't be needed if instead we did the root work outside this function instead of inside it. That way we don't have to branch inside the loop.

sebmarkbage · 2016-11-11T16:48:25Z

I worry about us making mutations directly to the tree. It makes it a bit difficult to reason about what will happen and what needs to be done to get the tree into a consistent state.

I think we should strive to reuse as much of the normal reconciliation logic as possible when we deal with errors. So that we know that everything gets processed in the same way.

I wonder if we can just do a normal reconciliation but force ReactChildFiber to not reuse the previous node. That way all the regular processing kicks in just as if children's key differ.

acdlite · 2016-11-11T20:24:55Z

Btw I'm aware there are some mistakes in my edits to ReactChildFiber. Fixing it up now (learning more about how it works as I go) but wanted to push this up to get overall feedback on the approach.

acdlite · 2016-11-12T01:36:35Z

I pushed some more changes. Implemented the failed root case using a flag on the FiberRoot. Added the concept of a root "error phase."

The SoftDeletion phase happens when the error is first caught, and indicates that the tree should be unmounted without removing any host nodes.
The HardDeletion phase indicates to ReactChildFiber that the next time the container is rendered into, the old nodes should be force removed from the DOM. This uses the normal reconciliation logic, as with the error boundary case.
The default phase is NoError.

These phases are only used for the failed root case. They're not necessary for the error boundary case because the nodes are immediately replaced by a new tree.

I'll wait for feedback before I add any more test coverage. Still not sure how to avoid a flag on the class instance (_isFailedErrorBoundary).

gaearon · 2016-11-12T11:30:47Z

Is activeErrorBoundaries.has() an adequate check for the case you implement with a flag? You could expose that to CommitWork too. If not maybe you could have a separate set for boundaries in that phase?

acdlite · 2016-11-12T17:46:15Z

@gaearon Yes I could thread a parameter through based on activeErrorBoundaries. It'd be a bit cumbersome because I need in inside ReactChildFiber, which is connected to the scheduler via ReactFiberBeginWork. I held off doing that at first to see if there was some way we could infer a failed error boundary without an extra field, or by explicitly passing an extra parameter. Don't think there is, though.

acdlite · 2016-11-13T17:59:11Z

src/renderers/shared/fiber/ReactFiberBeginWork.js

-      // continue from this point.
-      current.progressedChild = workInProgress.progressedChild;
-      current.progressedPriority = workInProgress.progressedPriority;
+  function BeginWork(forceReplace : boolean) {


The changes in this file are minimal; there's a lot of noise because I wrapped everything in this function, which increased the indentation.

The only real change is the addition of a forceReplace parameter.

acdlite · 2016-11-13T18:03:39Z

Latest commit removes the temporary _isFailedErrorBoundary hack. Good for another review.

@sebmarkbage I wrapped ReactFiberBeginWork in a wrapper function so that I could branch on whether the work in progress is a failed node, following the same pattern as ReactChildFiber. This seemed like a reasonable solution to me but perhaps I'm overlooking something.

outdated

When an error is thrown and caught by a boundary, we don't want to reuse the failed work when attempting to recover, because parts of the tree might be in an inconsistent state. Instead, we force rerender the entire tree from scratch. A similar thing happens with failed roots. When a tree fails with an uncaught error, the entire tree is force unmounted. But we don't want to actually remove the tree from the host environment, because otherwise the user would see an empty screen -- unlike the error boundary case, there's nothing to replace the failed tree with. So we do a "soft" delete, where componentWillUnmount is called recursively, but the host nodes remain. The next time something is rendered into the container, the old nodes are deleted.

This is a performance optimization and is unobservable. However, it helps protect against regressions on the following invariants on which it relies: - The priority of a fiber is greater than or equal to the priority of all its descendent fibers. - If a tree has pending work priority, its root is scheduled.

Added a new parameter to ReactChildFiber's wrapper function to force replace old nodes, effectively as if their keys don't match. The module exports a new function, replaceChildFibers, where this argument is true. ReactFiberBeginWork branches to use replaceChildFibers on failed nodes. To avoid excessive argument passing, I refactored ReactFiberBeginWork to use a wrapper function, like ReactChildFiber. It now returns beginWork and beginWorkOnFailedNode. The final bit of branching happens in the scheduler, which calls beginWorkOnFailedNode if the work in progress is a failed error boundary or a host container with an uncaught error.

sebmarkbage

Seems like this is waiting for #8304 so I'll flag it as needing changes or abandon.

gaearon · 2016-12-01T20:24:15Z

This was superseded by #8304.

acdlite assigned sebmarkbage and gaearon Nov 7, 2016

facebook-github-bot added the GH Review: review-needed label Nov 7, 2016

acdlite force-pushed the fiberunmountonerror branch 2 times, most recently from 9ec7825 to a7d0672 Compare November 7, 2016 23:04

facebook-github-bot added the CLA Signed label Nov 7, 2016

gaearon previously requested changes Nov 8, 2016

View reviewed changes

acdlite mentioned this pull request Nov 8, 2016

[Fiber] Should Fiber have a root field? #8222

Closed

This was referenced Nov 8, 2016

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

Closed

[Fiber] Error handling #8181

Closed

sebmarkbage mentioned this pull request Nov 8, 2016

[Fiber] Umbrella for remaining features / bugs #7925

Closed

acdlite force-pushed the fiberunmountonerror branch 2 times, most recently from f54a312 to 6834477 Compare November 11, 2016 01:24

gaearon previously requested changes Nov 11, 2016

View reviewed changes

gaearon reviewed Nov 11, 2016

View reviewed changes

sebmarkbage reviewed Nov 11, 2016

View reviewed changes

sebmarkbage added GH Review: needs-revision and removed GH Review: review-needed labels Nov 11, 2016

acdlite force-pushed the fiberunmountonerror branch 6 times, most recently from 6293042 to c7ad324 Compare November 12, 2016 01:20

acdlite force-pushed the fiberunmountonerror branch from f1dceb8 to 7bbe026 Compare November 12, 2016 01:45

acdlite force-pushed the fiberunmountonerror branch from ad217e3 to 1acd543 Compare November 13, 2016 17:57

acdlite commented Nov 13, 2016

View reviewed changes

acdlite force-pushed the fiberunmountonerror branch from 1acd543 to 2b2c189 Compare November 13, 2016 18:11

acdlite changed the title ~~[Fiber] If error is not caught, unmount entire tree~~ [Fiber] Force rerender a failed tree Nov 13, 2016

acdlite force-pushed the fiberunmountonerror branch 2 times, most recently from edc7cdb to 1758a0d Compare November 15, 2016 10:22

acdlite added 4 commits November 15, 2016 13:31

Fix setState callback error test

90f29a2

acdlite force-pushed the fiberunmountonerror branch from 1758a0d to 90f29a2 Compare November 15, 2016 13:32

acdlite mentioned this pull request Nov 16, 2016

[Fiber] New error boundary semantics #8304

Merged

3 tasks

sebmarkbage added GH Review: needs-revision and removed GH Review: review-needed labels Nov 22, 2016

sebmarkbage requested changes Nov 22, 2016

View reviewed changes

gaearon closed this Dec 1, 2016

		@@ -193,11 +195,14 @@ module.exports = function<T, P, I, TI, C>(
		commitNestedUnmounts(node);

		@@ -753,27 +753,31 @@ module.exports = function<T, P, I, TI, C>(config : HostConfig<T, P, I, TI, C>) {
		priorityLevel = TaskPriority;
		}

[Fiber] Force rerender a failed tree #8227

[Fiber] Force rerender a failed tree #8227

Uh oh!

Conversation

acdlite commented Nov 7, 2016

Uh oh!

sebmarkbage commented Nov 7, 2016

Uh oh!

gaearon commented Nov 8, 2016

Uh oh!

gaearon left a comment

Choose a reason for hiding this comment

Uh oh!

acdlite commented Nov 8, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gaearon commented Nov 8, 2016

Uh oh!

acdlite commented Nov 8, 2016

Uh oh!

sebmarkbage commented Nov 8, 2016

Uh oh!

acdlite commented Nov 8, 2016

Uh oh!

gaearon commented Nov 8, 2016

Uh oh!

acdlite commented Nov 11, 2016

Uh oh!

gaearon left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sebmarkbage commented Nov 11, 2016

Uh oh!

acdlite commented Nov 11, 2016

Uh oh!

acdlite commented Nov 12, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gaearon commented Nov 12, 2016

Uh oh!

acdlite commented Nov 12, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

acdlite commented Nov 13, 2016

Uh oh!

sebmarkbage left a comment

Choose a reason for hiding this comment

Uh oh!

gaearon commented Dec 1, 2016

Uh oh!

acdlite commented Nov 8, 2016 •

edited

Loading

acdlite commented Nov 12, 2016 •

edited

Loading

acdlite commented Nov 12, 2016 •

edited

Loading