Skip to content
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.

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

Add ReactInstrumentation #6068

Merged
merged 1 commit into from Feb 26, 2016
Merged

Add ReactInstrumentation #6068

merged 1 commit into from Feb 26, 2016

Conversation

gaearon
Copy link
Collaborator

@gaearon gaearon commented Feb 18, 2016

Summary

This adds ReactInstrumentation for the isomorphic package that uses the same approach as ReactDOMInstrumentation. Currently it is gated behind __DEV__ but we will likely change this later to a runtime flag determined by whether there are any active listeners.

The first few events we add here should be sufficient for React DevTools, as determined by the hook.emit() calls in https://github.com/facebook/react-devtools/blob/d90c43201617d5aef378669da926d79515b6a811/backend/attachRenderer.js.

These events will also be useful for reconstructing the parent tree in the ReactPerf rewrite in #6046.

Test Plan

Add this code to ReactDebugTool.js:

ReactDebugTool.addDevtool({
  onMountRootComponent(i) {
    console.log('onMountRootComponent', i);
  },
  onMountComponent(i) {
    console.log('onMountComponent', i);
  },
  onUpdateComponent(i) {
    console.log('onUpdateComponent', i);
  },
  onUnmountComponent(i) {
    console.log('onUnmountComponent', i);
  },
});

Run a React app and see that all calls are correctly reflected in the console.

Reviewers

@sebmarkbage @jimfb

This adds `ReactInstrumentation` for the isomorphic package that uses the same approach as `ReactDOMInstrumentation`. Currently it is gated behind `__DEV__` but we will likely change this later to a runtime flag determined by whether there are any active listeners.

The first few events we add here should be sufficient for React DevTools, as determined by the `hook.emit()` calls in https://github.com/facebook/react-devtools/blob/d90c43201617d5aef378669da926d79515b6a811/backend/attachRenderer.js.

These events will also be useful for reconstructing the parent tree in the ReactPerf rewrite in #6046.
@@ -347,6 +348,10 @@ var ReactMount = {
var wrapperID = componentInstance._instance.rootID;
instancesByReactRootID[wrapperID] = componentInstance;

if (__DEV__) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I put these behind __DEV__ for now but we’ll reconsider before merging #6046.

},
};

module.exports = ReactDebugTool;
Copy link
Contributor

Choose a reason for hiding this comment

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

We are going to need a debug tool specific to each renderer. Does it make sense to also have a shared isomorphic ReactDebugTool for just a few lifecycle mehods? Especially when some of the lifecycles may not exist for some renderers (eg. ServerSideRendering, if/when that gets split out). Intuitively, it feels simpler/cleaner if all the debug tools just always live in the individual renderers.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I was acting on @sebmarkbage’s comment in #6046 (comment).

ReactDOMInstrumentation is only for DOM specific operations so you'd want to replicate it with a ReactInstrumentation file that does the same thing.

We can definitely do it per renderer (especially considering that for now, we’ll have to do profiling calls in #6046 per renderer anyway). However even for profiling calls the longer term is to have shared reconciliation logic and do them in generic way, per #6046 (comment):

However, note that I also want to unify the way native components are rendered so that a lot of that wouldn't vary by renderer eventually. The recursion will move into shared code. That way we could put these measurements in a generic way.

The tools that rely on ReactInstrumentation will have to make some assumptions. I think that “components get mounted, updated, and unmounted” is a good one. Yes, it means that these hooks never fire on the server, but what practical difference does it make? On the other hand, as a benefit React DevTools extension can work with any renderer out of the box (pretty much like it does now, but without the monkey patching).

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, I have no strong objections. I think it's a little unfortunate to have two places to install the subtally-different debug tools, which is my main complaint, but I'm willing to defer to @sebmarkbage.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it makes sense if you think about what making a refactor would mean for the normal algorithm. These methods are required to exist. If we add an event to isomorphic code, then that would break every other renderer (as in ART, DOM, RN) out there until they get added. If they're part of isomorphic code then it doesn't break. It will just emit unknown events to the handlers.

@sebmarkbage
Copy link
Collaborator

@gaearon I think that you'll probably want the perf tools to BE the DebugTool. The point of this two level indirection is exactly for the perf tooling use case.

The perf tool can inject itself before the overhead of the event emitter kicks in. You can also stop the timer before calling any subsequent callbacks on other devtools that are attached, such as something keeping track of the structure full React tree.

This becomes especially important for many short operations since otherwise the overhead of the instrumentation will show up as artificially inflated time.

For example, you could have the perf tool be a DebugTool with a separate DevTool attached that tracks the tree structure.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants