[Fiber] Initial implementation of context #8272

gaearon · 2016-11-11T21:22:03Z

Known issues:

Passing nextContext to componentDidUpdate() would require changing the approach and adding a field to Fiber. For now we decided to bail on supporting this use case (e.g. Facebook doesn't have a single component in thousands using this pattern). We'll probably make a breaking change in Stack as well for this in 16.
Deep updates blocked by sCU still don't work: this is about feature parity.
It might be broken with error boundaries. I will address this in later PRs.

gaearon · 2016-11-11T21:23:32Z

scripts/fiber/tests-failing.txt

@@ -22,8 +22,7 @@ src/addons/transitions/__tests__/ReactTransitionGroup-test.js
 * should warn for duplicated keys with component stack info

 src/isomorphic/classic/__tests__/ReactContextValidator-test.js
-* should filter out context not in contextTypes
-* should filter context properly in callbacks
+* should pass previous context to lifecycles


This is a new PR I extracted from "should filter context properly in callbacks". It covers the componentDidUpdate with nextContext which we punted on.

🙄 I meant test, not PR

edvinerikson · 2016-11-12T09:02:53Z

This looks much cleaner than my approach, awesome 😄

gaearon · 2016-11-12T11:24:03Z

src/renderers/shared/fiber/ReactFiberBeginWork.js

@@ -182,11 +189,14 @@ module.exports = function<T, P, I, TI, C>(
    } else {
      shouldUpdate = updateClassInstance(current, workInProgress);
    }
+    const instance = workInProgress.stateNode;
+    if (typeof instance.getChildContext === 'function') {
+      pushContextProvider(workInProgress);


I should probably do this after rendering. Otherwise context provider would likely see its own context.

gaearon · 2016-11-14T00:44:48Z

Moved the implementation into scheduler to solve a bug with reusing work.
You can follow along in the commits but the final result may be easier to look at as a full PR.

We mistakingly forget to push the context if the parent is being reused but the child is not.

It is error-prone to push and pop context in begin or complete phases because of all the bailouts. Scheduler has enough knowledge to see if pushing is necessary because it knows when we go inside or outside the tree.

sebmarkbage · 2016-11-14T14:27:38Z

src/renderers/shared/fiber/ReactFiberScheduler.js

@@ -267,6 +274,11 @@ module.exports = function<T, P, I, TI, C>(config : HostConfig<T, P, I, TI, C>) {
      const current = workInProgress.alternate;
      const next = completeWork(current, workInProgress);

+      // We are leaving this subtree, so pop context if any.
+      if (isContextProvider(workInProgress)) {
+        popContextProvider();


I was hoping that we would be able to put push/pop inside ReactFiberBeginWork/ReactFiberCompleteWork. The reason is that I'd like to be able to reuse the tag check inside the switch statement so that we only even check for this if we hit the ClassComponent branch. That way when we build optimized paths, they can completely skip this check.

I realize that this might be currently broken because we bail out before the switch statement in certain cases right now. We might have to move those around.

sebmarkbage · 2016-11-14T16:09:12Z

src/renderers/shared/fiber/ReactFiberContext.js

+    checkReactTypeSpec(childContextTypes, childContext, 'childContext', name, null, debugID);
+  }
+
+  const mergedContext = Object.assign({}, getUnmaskedContext(), childContext);


Can we use spread with our Object.assign rewrite?

As per discussion, this is better because then this code only runs for the class type of work. We will still need to fix bailouts for deep setState calls.

acdlite · 2016-11-15T09:59:26Z

src/renderers/shared/fiber/ReactFiberContext.js

+
+let index = -1;
+const contextStack : Array<Object> = [];
+const didPerformWorkStack : Array<boolean> = [];


Could these arrays be combined into a single linked list? From what I can tell it doesn't look like you need indexing because you only ever need to access the tail.

In this case it'll (probably) be more efficient to use an array since we won't need to reorder anything and we won't need to reallocate anything when it grows and shrinks.

We can avoid recreating the merged context object if the context provider work was reused. This is essential to avoid allocations for deep setState() calls.

sebmarkbage · 2016-11-15T16:14:57Z

src/renderers/shared/fiber/ReactFiberTreeReflection.js

+    type.name || (constructor && constructor.name) ||
+    'A Component'
+  );
+};


We already have the getComponentName module that you could use:

https://github.com/facebook/react/blob/master/src/shared/utils/getComponentName.js#L18-L38

I tried but it was intentionally DEV-only at the time, and I need it for an invariant.
If we're ok increasing non-Fiber size with Fiber code before Fiber is ready, I could remove the DEV gate there.

sebmarkbage · 2016-11-15T16:16:57Z

src/renderers/shared/fiber/ReactFiberCompleteWork.js

@@ -125,6 +129,10 @@ module.exports = function<T, P, I, TI, C>(config : HostConfig<T, P, I, TI, C>) {
        return null;
      case ClassComponent:
        transferOutput(workInProgress.child, workInProgress);
+        // We are leaving this subtree, so pop context if any.
+        if (isContextProvider(workInProgress)) {
+          popContextProvider();


If we unwind to catch an error, we need to unwind the context as well so that we can continue with the next sibling after the error boundary.

I was thinking instead of literally unwinding to catch an error, we schedule an update on the boundary and reset the unit of work pointer. That's how I've started to implement it, not totally sure if it will work yet.

Where by "schedule an update" I mean give it work priority

This is not relevant now though, is it?

What I'm trying to say is that if we always start from the root, I don't think unwinding the context is necessary because of your line here: https://github.com/facebook/react/pull/8272/files#diff-74dedebbf08d8dcec2e92b8931274454R377

Yea that could work.

This matches Stack behavior. Had to rewrite a test that used Simulate because Fiber doesn't support events yet. Also changed tests for componentDidUpdate() since Fiber intentionally doesn't pass prevContext argument.

Faradey27 · 2016-11-15T22:10:44Z

src/renderers/shared/fiber/ReactFiberBeginWork.js

@@ -249,13 +262,15 @@ module.exports = function<T, P, I, TI, C>(
    }
    var fn = workInProgress.type;
    var props = workInProgress.pendingProps;
+    var context = getMaskedContext(workInProgress);
+
    var value;

    if (__DEV__) {


what the reason of such style?
why here you can't just have

var value = fn(props, context); if (__DEV__) { ReactCurrentOwner.current = workInProgress; }

or

if (__DEV__) { ReactCurrentOwner.current = workInProgress; } var value = fn(props, context);

I don't think there's any reason for this, putting just current owner assignment in a condition would be equivalent.

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

gaearon assigned sebmarkbage Nov 11, 2016

gaearon commented Nov 11, 2016

View reviewed changes

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

gaearon commented Nov 12, 2016

View reviewed changes

gaearon force-pushed the fiber-context branch from 32846c1 to 3947531 Compare November 13, 2016 23:04

gaearon added 7 commits November 14, 2016 13:29

Implement basic support for context

12bee76

Minor style tweaks

a05d1ab

Update passing tests

b8cca37

Fix bad ops.length assignment in tests

e6a0de5

Add a test for recursive context

4ba0eb9

Add a failing test for context when reusing work

c22b7a0

We mistakingly forget to push the context if the parent is being reused but the child is not.

Move context management into scheduler

f9e1bc9

It is error-prone to push and pop context in begin or complete phases because of all the bailouts. Scheduler has enough knowledge to see if pushing is necessary because it knows when we go inside or outside the tree.

gaearon force-pushed the fiber-context branch from f273275 to f9e1bc9 Compare November 14, 2016 13:32

sebmarkbage reviewed Nov 14, 2016

View reviewed changes

gaearon force-pushed the fiber-context branch from e7ed64c to 9ab904a Compare November 14, 2016 16:21

Move context handling back into Begin and Complete phases

56e1c12

As per discussion, this is better because then this code only runs for the class type of work. We will still need to fix bailouts for deep setState calls.

gaearon force-pushed the fiber-context branch from 9ab904a to 56e1c12 Compare November 14, 2016 16:22

Disable memoized props bailout when context might have changed

eedca6f

acdlite reviewed Nov 15, 2016

View reviewed changes

gaearon added 4 commits November 15, 2016 13:23

Add test cases for context below and above setState

299009c

Memoize merged child context when possible

a6ee5b8

We can avoid recreating the merged context object if the context provider work was reused. This is essential to avoid allocations for deep setState() calls.

Update passing tests

fd8c803

Add explicit tests for intermediate components

1e42c18

sebmarkbage reviewed Nov 15, 2016

View reviewed changes

Use empty object if context does not exist

2397f1f

This matches Stack behavior. Had to rewrite a test that used Simulate because Fiber doesn't support events yet. Also changed tests for componentDidUpdate() since Fiber intentionally doesn't pass prevContext argument.

sebmarkbage added the GH Review: accepted label Nov 15, 2016

sebmarkbage removed the GH Review: review-needed label Nov 15, 2016

sebmarkbage mentioned this pull request Nov 15, 2016

[Fiber] Umbrella for remaining features / bugs #7925

Closed

gaearon merged commit 4029ccd into facebook:master Nov 15, 2016

gaearon deleted the fiber-context branch November 15, 2016 17:14

Faradey27 reviewed Nov 15, 2016

View reviewed changes

acdlite mentioned this pull request Nov 17, 2016

[Fiber] New error boundary semantics #8304

Merged

3 tasks

[Fiber] Initial implementation of context #8272

[Fiber] Initial implementation of context #8272

Uh oh!

Conversation

gaearon commented Nov 11, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

edvinerikson commented Nov 12, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gaearon commented Nov 14, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

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!

Uh oh!

acdlite Nov 15, 2016 •

edited

Loading