[Fiber] Support React DevTools by gaearon · Pull Request #8818 · facebook/react

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

TODO:

This adds initial support for React DevTools for Fiber.
The corresponding React DevTools PR is facebook/react-devtools#475.

Historical notes

I initially tried to implement the diffing on React side (#8807), but after discussion with the team, decided against it to satisfy these constraints:

We want to keep it possible to debug websites in production which means code has to live elsewhere.
We don't know what kind of changes we'll want to do in DevTools to support Fiber-specific features (e.g. coroutines) so transport format is likely to change anyway.

If the architecture seems fine we should be able to merge this PR, and keep iterating on facebook/react-devtools#475 separately. The idea is that:

We notify DevTools of each commit (otherwise we can’t accurately diff current and WIP fibers).
We keep track of all mounted roots so that DevTools can attach later. When they attach, we synchronously call their handlers with the current trees of the mounted roots, and subscribe them.

gaearon · 2017-01-17T22:23:46Z

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


Looks like I'll have to move this back to the renderer.
It will need access to the component tree for "jump to component based on DOM node" feature.

sebmarkbage · 2017-01-17T22:45:37Z


 src/renderers/shared/hooks/__tests__/ReactComponentTreeHook-test.js
 * can be retrieved by ID
+* works


Why is this now failing? It seems like you didn't do anything to disable existing behavior.

Test is now erroring with TypeError: Set is not a constructor whereas before it was just failing with an expectDev

I use Set in DEV mode earlier so the test that specifically removed global.Set now fails. We use Set in other places anyway so I think this doesn’t really matter. We should fix those together (or depend on a polyfill).

sebmarkbage · 2017-01-17T23:37:49Z

+  var warning = require('warning');
+
+  var devCommitListeners = [];
+  var devMountedRoots = new Set();


This is unfortunate because it removes a neat GC clean up opportunity (although many subscriptions will hold onto the tree regardless). At least if a whole subtree can be GC:ed, right now we get the clean up automatically (if someone forgets unmountComponentAtNode).

The __REACT_DEVTOOLS_GLOBAL_HOOK__ is always available regardless if the tool is available or not. That gives us an opportunity to react to its existence. We know that we'll never need to drop any roots because we can just pass them along to the __REACT_DEVTOOLS_GLOBAL_HOOK__ and it can decide whether we need to keep them around for when the devtools starts.

The REACT_DEVTOOLS_GLOBAL_HOOK is always available regardless if the tool is available or not.

Oh, that's neat! I didn't realize.

sebmarkbage · 2017-01-17T23:38:40Z

  var ReactDebugCurrentFiber = require('ReactDebugCurrentFiber');
+  var warning = require('warning');
+
+  var devCommitListeners = [];


In practice, since this only attaches to __REACT_DEVTOOLS_GLOBAL_HOOK__ there will only ever need to be one listener, which is the __REACT_DEVTOOLS_GLOBAL_HOOK__ itself. It might be simpler just do track it as const listener = __REACT_DEVTOOLS_GLOBAL_HOOK__ and call on it.

That way this source can be smaller for when we put it in prod.

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

Ready for another review.

bvaughn

This looks slick, Dan.

sebmarkbage · 2017-01-26T21:44:21Z

+      // This way older DevTools can ignore newer Fiber versions.
+      version: 1,
+    });
+  };


I tend to prefer putting exports outside the conditional at the top level so that we can easily convert this to ES modules. Also guarantees that all names are always exported and constant bindings.

sebmarkbage · 2017-01-26T21:47:44Z

+      __REACT_DEVTOOLS_GLOBAL_HOOK__.onCommitFiberRoot(rendererID, root);
+    } catch (err) {
+      // Catch all errors because it is unsafe to throw in the commit phase.
+      warning(false, 'React DevTools encountered an error: %s', err);


Aren't all warnings supposed to be __DEV__ only so we can strip the warning module and its entire system in PROD? This is available in PROD.

My apologies. I was unaware of this.

I think there are a couple of other places where we have warning outside of __DEV__ (eg ReactDOMFiberSelect). Should we clean these up?

I think the module itself is disabled in PROD and we should probably ensure that it gets fully stripped with a transform like we do for invariant messages.

It was just unclear what was intended here.

I enabled it in prod per Ben's feedback.

Oh, I didn't realize we are putting all warnings behind DEV though. Somehow I though Babel transform is doing it. I'll add a condition. (I think it's fine to crash DevTools silently when using DevTools in prod)

I can also move the try-catch to the DevTools.

sebmarkbage · 2017-01-26T21:55:13Z

+) {
+  exports.injectInternals = function(internals : Object) {
+    warning(rendererID == null, 'Cannot inject into DevTools twice.');
+    rendererID = __REACT_DEVTOOLS_GLOBAL_HOOK__.inject({


I wonder if we should capture __REACT_DEVTOOLS_GLOBAL_HOOK__.inject et all in constant closure variables to make it type safe and easily inlinable by the VM. Currently, it's not since both the global and the object itself could've been mutated.

sebmarkbage · 2017-01-26T22:20:15Z

+      ...internals,
+      // Increment when internals become incompatible with DevTools.
+      // This way older DevTools can ignore newer Fiber versions.
+      version: 1,


I'm not sure it makes sense to put the version here. 1) It adds runtime Object.assign cost and more code. Probably resulting in an unoptimized object atm. 2) The version indicates what API is exposed, but that API is defined by the DOM renderer here. If you bump this you have to make sure that all consumers and downstream renderers bump theirs too. So isn't the current version more associated with the DOM renderer rather than this shared helper?

I'm also skeptical about version numbers being super useful here anyway. Often we'll want to add features to the devtools that work with minor already released versions without the ability to bump this so we would and those features might not work with the whole range of the version.

So I think we'll probably just end up with feature detection anyway.

If you bump this you have to make sure that all consumers and downstream renderers bump theirs too.

Why? The way I imagined, every time we make an incompatible change, we can fork the code in DevTools (and eventually start removing old forks). The only difference is this is now explicit. The downstream renderers would update it at the same time they update their reconciler version (which is really what version field specifies).

My main intention is for old DevTools versions to not crash when they attach to new Fiber versions that, for example, have different children data structure. In the past, data structures barely changed, but I'm not sure this will be true for Fiber (e.g. hybernating trees). So my intention is that Fiber can say "hey, I'm the new incompatible version" and then old DevTools just bail rather than crash.

We can't feature test that because we can't modify old versions of DevTools once they're out. But maybe it doesn't matter because they auto update?

Another example of incompatibility is changing types of work enum. If old tools can't just bail they might display everything incorrectly and it wouldn't be obvious they need to be updated.

In your work enum case, it's similarly not obvious that you need to bump the version number when you make this change. Effectively, we'd just have to bump the version number all the time, meaning we also have to make devtools opt in all the time for every change even though most of them keeps working just fine.

And if we add features to devtools after the fact, we'd end up with things relying on internals anyway.

it's similarly not obvious that you need to bump the version number when you make this change

I'll catch you in code review! 😄
But yea, I see your point.

This is fine because we don't use it in Stack anyway (at least not that part), and we also rely on Set/Map in other parts of code. The newly failing test is the one saying "it works without Map/Set".

This lets us ignore pre-Fiber DevTools instead of crashing them by injecting a different API.

gaearon · 2017-01-27T22:50:31Z

I'll do RN and Art as a followup. I think this is ready to get in.

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

This was referenced Jan 17, 2017

[Fiber][WIP] Support React DevTools #8807

Closed

[WIP] Support and use Fiber facebook/react-devtools#473

Closed

Support Fiber facebook/react-devtools#475

Merged

gaearon changed the title ~~[Fiber][WIP] Support React DevTools (take two)~~ [Fiber] Support React DevTools (take two) Jan 17, 2017

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

gaearon commented Jan 17, 2017

View reviewed changes

sebmarkbage reviewed Jan 17, 2017

View reviewed changes

bvaughn self-assigned this Jan 18, 2017

gaearon force-pushed the fiber-devtools-2 branch 2 times, most recently from 2d2fab5 to ca38d3f Compare January 26, 2017 14:36

bvaughn approved these changes Jan 26, 2017

View reviewed changes

sebmarkbage reviewed Jan 26, 2017

View reviewed changes

gaearon added 9 commits January 27, 2017 18:49

Initial React DevTools support for Fiber

b1730ea

Record a newly failing irrelevant test

3648143

This is fine because we don't use it in Stack anyway (at least not that part), and we also rely on Set/Map in other parts of code. The newly failing test is the one saying "it works without Map/Set".

Explicitly check for Fiber DevTools support

c14e7ed

This lets us ignore pre-Fiber DevTools instead of crashing them by injecting a different API.

Track the roots in DevTools instead

c2c24bf

Lol Dan this is not how warnings work in our codebase

1ceaecc

Move injection to the renderer

39ba9ea

Notify DevTools about unmounts

168b80e

Make it work in production

f52d7ae

Fix lint and flow

11fc0e1

Fixups

5c2456e

gaearon force-pushed the fiber-devtools-2 branch from 39749c9 to 5c2456e Compare January 27, 2017 18:49

Address feedback

12f5736

gaearon force-pushed the fiber-devtools-2 branch from 2e84c1a to 12f5736 Compare January 27, 2017 21:12

gaearon changed the title ~~[Fiber] Support React DevTools (take two)~~ [Fiber] Support React DevTools Jan 27, 2017

gaearon merged commit d3e29d4 into facebook:master Jan 27, 2017

gaearon deleted the fiber-devtools-2 branch January 27, 2017 22:50

gaearon mentioned this pull request Feb 4, 2017

[Fiber] Inject DevTools integration for React Native #8930

Merged

Conversation

gaearon commented Jan 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Historical notes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bvaughn Jan 17, 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!

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!

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!

sebmarkbage 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

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 commented Jan 27, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

gaearon commented Jan 17, 2017 •

edited

Loading

bvaughn Jan 17, 2017 •

edited

Loading

gaearon Jan 18, 2017 •

edited

Loading

sebmarkbage Jan 26, 2017 •

edited

Loading