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
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
65 changes: 65 additions & 0 deletions src/isomorphic/ReactDebugTool.js
@@ -0,0 +1,65 @@
/**
* Copyright 2016-present, Facebook, Inc.
* All rights reserved.
*
* This source code is licensed under the BSD-style license found in the
* LICENSE file in the root directory of this source tree. An additional grant
* of patent rights can be found in the PATENTS file in the same directory.
*
* @providesModule ReactDebugTool
*/

'use strict';

var warning = require('warning');

var eventHandlers = [];
var handlerDoesThrowForEvent = {};

function emitEvent(handlerFunctionName, arg1, arg2, arg3, arg4, arg5) {
if (__DEV__) {
eventHandlers.forEach(function(handler) {
try {
if (handler[handlerFunctionName]) {
handler[handlerFunctionName](arg1, arg2, arg3, arg4, arg5);
}
} catch (e) {
warning(
!handlerDoesThrowForEvent[handlerFunctionName],
'exception thrown by devtool while handling %s: %s',
handlerFunctionName,
e.message
);
handlerDoesThrowForEvent[handlerFunctionName] = true;
}
});
}
}

var ReactDebugTool = {
addDevtool(devtool) {
eventHandlers.push(devtool);
},
removeDevtool(devtool) {
for (var i = 0; i < eventHandlers.length; i++) {
if (eventHandlers[i] === devtool) {
eventHandlers.splice(i, 1);
i--;
}
}
},
onMountRootComponent(internalInstance) {
emitEvent('onMountRootComponent', internalInstance);
},
onMountComponent(internalInstance) {
emitEvent('onMountComponent', internalInstance);
},
onUpdateComponent(internalInstance) {
emitEvent('onUpdateComponent', internalInstance);
},
onUnmountComponent(internalInstance) {
emitEvent('onUnmountComponent', internalInstance);
},
};

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.

16 changes: 16 additions & 0 deletions src/isomorphic/ReactInstrumentation.js
@@ -0,0 +1,16 @@
/**
* Copyright 2016-present, Facebook, Inc.
* All rights reserved.
*
* This source code is licensed under the BSD-style license found in the
* LICENSE file in the root directory of this source tree. An additional grant
* of patent rights can be found in the PATENTS file in the same directory.
*
* @providesModule ReactInstrumentation
*/

'use strict';

var ReactDebugTool = require('ReactDebugTool');

module.exports = {debugTool: ReactDebugTool};
5 changes: 5 additions & 0 deletions src/renderers/dom/client/ReactMount.js
Expand Up @@ -20,6 +20,7 @@ var ReactDOMContainerInfo = require('ReactDOMContainerInfo');
var ReactDOMFeatureFlags = require('ReactDOMFeatureFlags');
var ReactElement = require('ReactElement');
var ReactFeatureFlags = require('ReactFeatureFlags');
var ReactInstrumentation = require('ReactInstrumentation');
var ReactMarkupChecksum = require('ReactMarkupChecksum');
var ReactPerf = require('ReactPerf');
var ReactReconciler = require('ReactReconciler');
Expand Down Expand Up @@ -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.

ReactInstrumentation.debugTool.onMountRootComponent(componentInstance);
}

return componentInstance;
},

Expand Down
16 changes: 15 additions & 1 deletion src/renderers/shared/reconciler/ReactReconciler.js
Expand Up @@ -12,6 +12,7 @@
'use strict';

var ReactRef = require('ReactRef');
var ReactInstrumentation = require('ReactInstrumentation');

/**
* Helper to call ReactRef.attachRefs with this composite component, split out
Expand Down Expand Up @@ -51,6 +52,9 @@ var ReactReconciler = {
internalInstance._currentElement.ref != null) {
transaction.getReactMountReady().enqueue(attachRefs, internalInstance);
}
if (__DEV__) {
ReactInstrumentation.debugTool.onMountComponent(internalInstance);
}
return markup;
},

Expand All @@ -70,7 +74,10 @@ var ReactReconciler = {
*/
unmountComponent: function(internalInstance, safely) {
ReactRef.detachRefs(internalInstance, internalInstance._currentElement);
return internalInstance.unmountComponent(safely);
internalInstance.unmountComponent(safely);
if (__DEV__) {
ReactInstrumentation.debugTool.onUnmountComponent(internalInstance);
}
},

/**
Expand Down Expand Up @@ -119,6 +126,10 @@ var ReactReconciler = {
internalInstance._currentElement.ref != null) {
transaction.getReactMountReady().enqueue(attachRefs, internalInstance);
}

if (__DEV__) {
ReactInstrumentation.debugTool.onUpdateComponent(internalInstance);
}
},

/**
Expand All @@ -133,6 +144,9 @@ var ReactReconciler = {
transaction
) {
internalInstance.performUpdateIfNecessary(transaction);
if (__DEV__) {
ReactInstrumentation.debugTool.onUpdateComponent(internalInstance);
}
},

};
Expand Down