Support Fiber by gaearon · Pull Request #475 · facebook/react-devtools

gaearon · 2017-01-17T20:55:46Z

This works on top of facebook/react#8818.
We diff trees on every commit and create a list of events to send to the hook.

React Fiber will check this flag before attempting to inject itself. This avoids crashes on pre-Fiber DevTools version when Fiber is on the page.

sebmarkbage · 2017-01-17T23:07:55Z

+    const current = root.current;
+    const alternate = current.alternate;
+    const events = [];
+    if (alternate && subscription != null) {


What if you're attaching to an existing tree that already has an alternate?

Then subscription would be null (since we haven't finished subscribing, and the React side calls the commit hook synchronously during subscription the first time).

When React calls it the first time (synchronously), then there can already be an alternate though, right?

Right. But it's && so we would go into the other branch which always mounts.

sebmarkbage · 2017-01-17T23:13:52Z

+      const prevChild = prevChildren.get(key);
+      const nextChild = nextChildren.get(key);
+      if (prevChild != null && nextChild != null) {
+        // TODO: are there more cases when we can bail out?


Currently, either all children are cloned or none are cloned. So you really only need to check nextFiber.child === prevFiber.child to know if all are the same or all are different.

Is this an implementation detail that will hold true? I thought we might change this in the future as optimization but wasn't sure.

sebmarkbage · 2017-01-17T23:15:14Z

+    }
+  }
+
+  function enqueueUpdate(fiber, events) {


If props, state and the others have reference equality, you shouldn't need to push any events here. Helpful for deep updates since nothing along the path will have been updated.

I'd still need to compare children though, don't I? So that would be reference equality for everything except child ID array (which I'd compare shallowly).

sebmarkbage · 2017-01-17T23:21:16Z

+  function updateFiber(nextFiber, prevFiber, events) {
+    // TODO: optimize for the common case of children with implcit keys.
+    // Just like we do in the Fiber child reconciler.
+    // We shouldn't be allocating Maps all the time.


You shouldn't even need to diff on keys and types (which is incomplete anyway) since you can match on object equality between the fiber alternates.

If the first child is equal, then all the children are equal. If the first child is non-equal, you know that you will need to enqueue and update on everything that has an alternate and a mount on anything without an alternate.

The only thing you might need diffing for is unmounts. One solution is to just loop through all the next children with alternates. Add all the alternates to a Set. Then loop through all the previous children, if they're not in the Set, they're deleted.

A better solution might be to just expose a hook for deletions instead.

Nice, thanks for explaining!

You shouldn't even need to diff on keys and types (which is incomplete anyway)

Why is it incomplete though? What did I miss?

bvaughn · 2017-01-17T23:27:12Z

+  };
+}
+
+module.exports = getFiberData;


nit: File name is getDataFiber; exports function getFiberData. The latter sounds less awkward.

same thing with attachFiberRenderer.

We have existing getData013, getData012. I was striving for them all to appear together with their non-Fiber equivalents. But I don't care, can change for readability too.

(Didn't notice the inconsistency—probably because I changed it some point. Which way do you prefer?)

I'd maybe vote for consistency w/r/t getData12? so getDataFiber

gaearon · 2017-01-26T21:21:08Z

Ready for another review.

bvaughn

Some thoughts! 😄 Probably should be reviewed by at least one more person though.

bvaughn · 2017-01-26T21:56:42Z

+      element: getOpaqueNode(fiber),
+      data: getDataFiber(fiber, getOpaqueNode),
+      renderer: rid,
+      _event: 'mount',


nit: Why not type or _type (eg event.type seems more intuitive than event._event)

Mostly I wasn't sure if any downstream code handling these events gives any meaning to event.type or other property names. I'll check and rename. 😄

bvaughn · 2017-01-26T22:08:07Z

+    }
+    opaqueNodes.delete(fiber);
+    if (fiber.alternate != null) {
+      opaqueNodes.delete(fiber.alternate);


nit: If you stored the value value returned on line 127 you would only have to delete that (and could avoid checking fiber.alternate again).

bvaughn · 2017-01-26T22:11:54Z

+    let node = fiber;
+    outer: while (true) {
+      if (node.child) {
+        node.child.return = node;


Why does this function modify return pointers? That doesn't seem right. Am I reading this wrong? (Also when would node.child.return ever not equal node?)

In React source, we do this in all traversals to avoid inifinite loops. They happen if return points to alternate of the parent you're traversing.

I'm not sure if it's necessary here but I did it for extra safety. It should be fine but maybe @spicyj or @acdlite can verify.

They happen if return points to alternate of the parent you're traversing.

Wouldn't this only happen if there was a bug in fiber?

I expect these return assignments in devtools are basically neutral since the pointers should be correct to begin with, but it still seems weird to see something outside of React setting values on fibers.

Wouldn't this only happen if there was a bug in fiber?

I don't know if Fiber guarantees that returns will match up anywhere. Fiber itself always reassigns them when traversing so that's what I did. I understand it seems a bit fishy but I don't see any harm in this.

There should be no observable side effects to reassigning return I think. However, if return doesn't match, we risk getting an infinite loop.

It wouldn't be too bad to use recursion here instead if you wanted to avoid the concern and the implementation details of return. But then again, maybe this could should live colocated with the React Fiber source code so it feels less smelly.

Meh, we rely on implementation details a lot anyway. I think it's fine if you don't mind. I don't want to further bloat the bundle since we decided to enable this in prod.

bvaughn · 2017-01-26T22:21:36Z

+      _event: 'mount',
+    });
+
+    const isRoot = fiber.tag === 3;


nit: Because of references like this and the ones in getDataFiber I wonder if it wouldn't be clearer to define a duplicate ReactTypeOfWork const in this project. At least this way if a value ever changes it's clear where all of the mirrored values are (and we aren't just greping for numbers).

bvaughn · 2017-01-26T22:24:25Z

+          context = null;
+        }
+      }
+      const inst = publicInstance;


Why is the inst var needed?

To avoid next lines breaking over 80 character lint :-)
I can restructure.

Fair enough! 🤣

sebmarkbage · 2017-01-26T23:12:03Z

        }
      },
+      // Fiber-only:
+      supportsFiber: true,


Is this unused now?

No:

if ( typeof __REACT_DEVTOOLS_GLOBAL_HOOK__ !== 'undefined' && typeof __REACT_DEVTOOLS_GLOBAL_HOOK__.supportsFiber ) {

Necessary so that Fiber doesn't try to attach to non-Fiber DevTools and crash them.

If we release a new version of the devtools that auto-updates everyone then before we release Fiber we can revert this check with the assumption that devtools gets updated before renderers?

Yes I think so.

sebmarkbage · 2017-01-26T23:39:14Z

+        }
+      },
+      onCommitFiberRoot: function(rendererID, root) {
+        const mountedRoots = this.getFiberRoots(rendererID);


It's unfortunate that the use of this instead of closure capture invalidates the suggestion I had in https://github.com/facebook/react/pull/8818/files#r98101132

sebmarkbage · 2017-01-26T23:54:59Z

+        const mountedRoots = this.getFiberRoots(rendererID);
+        const current = root.current;
+        const isKnownRoot = mountedRoots.has(root);
+        const isUnmounting = current.memoizedState == null || current.memoizedState.element == null;


cc @acdlite This is regard to roots but I don't really have a better idea.

We could possibly make onCommitFiberRoot accept the direct children of roots instead of the actual root but it's nice to be able to have the mount container target here too for various debug data.

sebmarkbage · 2017-01-27T01:35:33Z

+    const prevData = getDataFiber(fiber.alternate, getOpaqueNode);
+    // Avoid unnecessary updates since they are common
+    // when something changes deep in the tree due to setState.
+    if (!hasDataChanged(prevData, nextData)) {


Would it be better to do this at the fiber level instead? Maybe it's more fragile to updates but faster since you don't have to create the other objects.

Addressed by 09517f2.

sebmarkbage

There's a bunch of nits but I think conceptually this is a good start to land.

sebmarkbage · 2017-01-27T04:10:05Z

+      prevData.ref !== nextData.ref ||
+      prevData.source !== nextData.source ||
+      prevData.props !== nextData.props ||
+      prevData.state !== nextData.state ||


During a forceUpdate I don't think state object reference has necessarily changed and yet the component can have been mutated deep below.

cc @acdlite

So, we need to know this information for life-cycles too. Currently we schedule an update effect tag that will be consumed during the commit. You might be able to use that. Ideally we would only do that if there exists a componentDidUpdate life-cycle but maybe we can keep doing that for this purpose too.

In 09517f2, I check .updateQueue.hasForcedUpdate on the committed work and it seems to be fine. Can I rely on this?

Instead of diffing the data, we diff the fibers themselves. We also keep track of child order changes so that we don't have to diff children separately. This lets us bail out more efficiently.

It is necessary in cases we get a different fiber than the one we started with.

* Initial support for Fiber * Set a flag indicating Fiber-capable DevTools React Fiber will check this flag before attempting to inject itself. This avoids crashes on pre-Fiber DevTools version when Fiber is on the page. * Rebuild * Specify the missing key * Make function naming consistent * Attach to the tree lazily * Implement DOM selection * Implement updater for composites * Avoid diffing children * Support portals * Rebuild * Fix lint and flow * The bridge is async so we may receive unmounted fibers * Fix comment * Compare fibers more efficiently Instead of diffing the data, we diff the fibers themselves. We also keep track of child order changes so that we don't have to diff children separately. This lets us bail out more efficiently. * _event => type * Simplify unmounting code * Copy ReactTypeOfWork enum * Address feedback * Add opaque node lookup to getReactElementFromNative() It is necessary in cases we get a different fiber than the one we started with. * Fix Flow

Initial support for Fiber

4a5b458

facebook-github-bot added CLA Signed GH Review: review-needed labels Jan 17, 2017

gaearon mentioned this pull request Jan 17, 2017

[Fiber] Support React DevTools facebook/react#8818

Merged

12 tasks

gaearon force-pushed the fiber-3 branch from b1b2637 to 4a5b458 Compare January 17, 2017 20:57

gaearon changed the title ~~[WIP] Initial support for Fiber~~ Initial (incomplete) support for Fiber Jan 17, 2017

gaearon requested review from sebmarkbage and sophiebits January 17, 2017 20:59

gaearon added 2 commits January 17, 2017 21:11

Set a flag indicating Fiber-capable DevTools

0e38f59

React Fiber will check this flag before attempting to inject itself. This avoids crashes on pre-Fiber DevTools version when Fiber is on the page.

Rebuild

ad8d650

sebmarkbage reviewed Jan 17, 2017

View reviewed changes

bvaughn reviewed Jan 17, 2017

View reviewed changes

gaearon added 5 commits January 24, 2017 18:55

Specify the missing key

d427de7

Make function naming consistent

718e992

Attach to the tree lazily

c17c6c9

Implement DOM selection

acf7e5f

Implement updater for composites

2e9aef7

gaearon force-pushed the fiber-3 branch from a281711 to 2e9aef7 Compare January 26, 2017 14:35

Avoid diffing children

4ac6d6e

gaearon force-pushed the fiber-3 branch from 5ce6e95 to 4ac6d6e Compare January 26, 2017 18:13

gaearon added 4 commits January 26, 2017 18:38

Support portals

a883f89

Rebuild

01c029c

Fix lint and flow

0558aae

The bridge is async so we may receive unmounted fibers

f43c273

gaearon changed the title ~~Initial (incomplete) support for Fiber~~ Support Fiber Jan 26, 2017

Fix comment

504aa5e

bvaughn reviewed Jan 26, 2017

View reviewed changes

sebmarkbage reviewed Jan 26, 2017

View reviewed changes

sebmarkbage reviewed Jan 27, 2017

View reviewed changes

sebmarkbage approved these changes Jan 27, 2017

View reviewed changes

sebmarkbage reviewed Jan 27, 2017

View reviewed changes

gaearon force-pushed the fiber-3 branch from 040ca22 to 040eaa0 Compare January 27, 2017 20:02

Compare fibers more efficiently

09517f2

Instead of diffing the data, we diff the fibers themselves. We also keep track of child order changes so that we don't have to diff children separately. This lets us bail out more efficiently.

gaearon force-pushed the fiber-3 branch from 040eaa0 to 09517f2 Compare January 27, 2017 20:05

gaearon added 6 commits January 27, 2017 20:18

_event => type

f5229f2

Simplify unmounting code

a379220

Copy ReactTypeOfWork enum

97a8fff

Address feedback

5a1777f

Add opaque node lookup to getReactElementFromNative()

03e8da2

It is necessary in cases we get a different fiber than the one we started with.

Fix Flow

8dfedaa

gaearon merged commit 4f6bec0 into master Jan 27, 2017

Conversation

gaearon commented Jan 17, 2017 • 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!

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!

gaearon Jan 18, 2017 • 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!

gaearon commented Jan 26, 2017

Uh oh!

bvaughn 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!

bvaughn Jan 26, 2017 • 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!

gaearon Jan 27, 2017 • 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!

bvaughn Jan 26, 2017 • 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

gaearon commented Jan 17, 2017 •

edited

Loading

gaearon Jan 18, 2017 •

edited

Loading

bvaughn Jan 26, 2017 •

edited

Loading

gaearon Jan 27, 2017 •

edited

Loading

bvaughn Jan 26, 2017 •

edited

Loading

sebmarkbage Jan 27, 2017 •

edited

Loading