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.

Sign up for GitHub

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

Track source more generically in ReactComponentTreeDevtool #6765

Merged
merged 1 commit into from
May 14, 2016
Merged

Track source more generically in ReactComponentTreeDevtool #6765

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions src/isomorphic/ReactDebugTool.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -241,10 +241,18 @@ var ReactDebugTool = {
checkDebugID(debugID);
emitEvent('onMountRootComponent', debugID);
},
onBeforeMountComponent(debugID, element) {
checkDebugID(debugID);
emitEvent('onBeforeMountComponent', debugID, element);
},
onMountComponent(debugID) {
checkDebugID(debugID);
emitEvent('onMountComponent', debugID);
},
onBeforeUpdateComponent(debugID, element) {
checkDebugID(debugID);
emitEvent('onBeforeUpdateComponent', debugID, element);
},
onUpdateComponent(debugID) {
checkDebugID(debugID);
emitEvent('onUpdateComponent', debugID);
Expand Down
16 changes: 16 additions & 0 deletions src/isomorphic/devtools/ReactComponentTreeDevtool.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ var rootIDs = [];
function updateTree(id, update) {
if (!tree[id]) {
tree[id] = {
element: null,
parentID: null,
ownerID: null,
text: null,
Expand Down Expand Up @@ -88,6 +89,14 @@ var ReactComponentTreeDevtool = {
updateTree(id, item => item.text = text);
},

onBeforeMountComponent(id, element) {
updateTree(id, item => item.element = element);
},

onBeforeUpdateComponent(id, element) {
updateTree(id, item => item.element = element);
},

onMountComponent(id) {
updateTree(id, item => item.isMounted = true);
},
Expand Down Expand Up @@ -141,6 +150,13 @@ var ReactComponentTreeDevtool = {
return item ? item.parentID : null;
},

getSource(id) {
var item = tree[id];
var element = item ? item.element : null;
var source = element != null ? element._source : null;
return source;
},

getText(id) {
var item = tree[id];
return item ? item.text : null;
Expand Down
9 changes: 0 additions & 9 deletions src/renderers/dom/shared/ReactDOMComponent.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,6 @@ var ReactDOMButton = require('ReactDOMButton');
var ReactDOMComponentFlags = require('ReactDOMComponentFlags');
var ReactDOMComponentTree = require('ReactDOMComponentTree');
var ReactDOMInput = require('ReactDOMInput');
var ReactDOMInstrumentation = require('ReactDOMInstrumentation');
var ReactDOMOption = require('ReactDOMOption');
var ReactDOMSelect = require('ReactDOMSelect');
var ReactDOMTextarea = require('ReactDOMTextarea');
Expand Down Expand Up @@ -579,10 +578,6 @@ ReactDOMComponent.Mixin = {
validateDOMNesting.updatedAncestorInfo(parentInfo, this._tag, this);
}

if (__DEV__) {
ReactDOMInstrumentation.debugTool.onMountDOMComponent(this._debugID, this._currentElement);
}

var mountImage;
if (transaction.useCreateElement) {
var ownerDocument = hostContainerInfo._ownerDocument;
Expand Down Expand Up @@ -858,10 +853,6 @@ ReactDOMComponent.Mixin = {
context
);

if (__DEV__) {
ReactDOMInstrumentation.debugTool.onUpdateDOMComponent(this._debugID, this._currentElement);
}

if (this._tag === 'select') {
// <select> value update needs to occur after <option> children
// reconciliation
Expand Down
9 changes: 3 additions & 6 deletions src/renderers/dom/shared/ReactDOMDebugTool.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
'use strict';

var ReactDOMUnknownPropertyDevtool = require('ReactDOMUnknownPropertyDevtool');
var ReactDebugTool = require('ReactDebugTool');

var warning = require('warning');

Expand Down Expand Up @@ -40,9 +41,11 @@ function emitEvent(handlerFunctionName, arg1, arg2, arg3, arg4, arg5) {

var ReactDOMDebugTool = {
addDevtool(devtool) {
ReactDebugTool.addDevtool(devtool);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Are we going to expose just one of them in the end?

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 figured we would probably expose both – though maybe it makes more sense to only expose each platform-specific one. Open to other suggestions on how to set ReactDOMUnknownPropertyDevtool up so that we can listen to both kinds of events though.

Copy link
Collaborator

Choose a reason for hiding this comment

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

maybe it makes more sense to only expose each platform-specific one

Let’s go with this assumption for now. In this case it makes sense to automatically setup forwarding.

eventHandlers.push(devtool);
},
removeDevtool(devtool) {
ReactDebugTool.removeDevtool(devtool);
for (var i = 0; i < eventHandlers.length; i++) {
if (eventHandlers[i] === devtool) {
eventHandlers.splice(i, 1);
Expand All @@ -62,12 +65,6 @@ var ReactDOMDebugTool = {
onTestEvent() {
emitEvent('onTestEvent');
},
onMountDOMComponent(debugID, element) {
emitEvent('onMountDOMComponent', debugID, element);
},
onUpdateDOMComponent(debugID, element) {
emitEvent('onMountDOMComponent', debugID, element);
},
};

ReactDOMDebugTool.addDevtool(ReactDOMUnknownPropertyDevtool);
Expand Down
19 changes: 16 additions & 3 deletions src/renderers/dom/shared/__tests__/ReactDOMComponent-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1306,8 +1306,17 @@ describe('ReactDOMComponent', function() {
ReactDOMServer.renderToString(<div class="paladin"/>);
ReactDOMServer.renderToString(<input type="text" onclick="1"/>);
expect(console.error.argsForCall.length).toBe(2);
expect(console.error.argsForCall[0][0]).toMatch(/.*className.*\(.*:\d+\)/);
expect(console.error.argsForCall[1][0]).toMatch(/.*onClick.*\(.*:\d+\)/);
expect(
console.error.argsForCall[0][0].replace(/\(.+?:\d+\)/g, '(**:*)')
).toBe(
'Warning: Unknown DOM property class. Did you mean className? (**:*)'
);
expect(
console.error.argsForCall[1][0].replace(/\(.+?:\d+\)/g, '(**:*)')
).toBe(
'Warning: Unknown event handler property onclick. Did you mean ' +
'`onClick`? (**:*)'
);
});

it('gives source code refs for unknown prop warning for update render', function() {
Expand All @@ -1319,7 +1328,11 @@ describe('ReactDOMComponent', function() {

ReactDOMServer.renderToString(<div class="paladin" />, container);
expect(console.error.argsForCall.length).toBe(1);
expect(console.error.argsForCall[0][0]).toMatch(/.*className.*\(.*:\d+\)/);
expect(
console.error.argsForCall[0][0].replace(/\(.+?:\d+\)/g, '(**:*)')
).toBe(
'Warning: Unknown DOM property class. Did you mean className? (**:*)'
);
});

it('gives source code refs for unknown prop warning for exact elements ', function() {
Expand Down
25 changes: 14 additions & 11 deletions src/renderers/dom/shared/devtools/ReactDOMUnknownPropertyDevtool.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,11 +13,12 @@

var DOMProperty = require('DOMProperty');
var EventPluginRegistry = require('EventPluginRegistry');
var ReactComponentTreeDevtool = require('ReactComponentTreeDevtool');

var warning = require('warning');

if (__DEV__) {
var cachedSource;
var lastDebugID;
var reactProps = {
children: true,
dangerouslySetInnerHTML: true,
Expand Down Expand Up @@ -47,14 +48,18 @@ if (__DEV__) {
null
);

var source = ReactComponentTreeDevtool.getSource(lastDebugID);
var formattedSource = source ?
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is it worth adding getFormattedSource() to ReactComponentTreeDevtool for convenience?

`(${source.fileName.replace(/^.*[\\\/]/, '')}:${source.lineNumber})` : '';

// For now, only warn when we have a suggested correction. This prevents
// logging too much when using transferPropsTo.
warning(
standardName == null,
'Unknown DOM property %s. Did you mean %s? %s',
name,
standardName,
formatSource(cachedSource)
formattedSource
);

var registrationName = (
Expand All @@ -70,14 +75,10 @@ if (__DEV__) {
'Unknown event handler property %s. Did you mean `%s`? %s',
name,
registrationName,
formatSource(cachedSource)
formattedSource
);
};

var formatSource = function(source) {
return source ? `(${source.fileName.replace(/^.*[\\\/]/, '')}:${source.lineNumber})` : '';
};

}

var ReactDOMUnknownPropertyDevtool = {
Expand All @@ -90,11 +91,13 @@ var ReactDOMUnknownPropertyDevtool = {
onDeleteValueForProperty(node, name) {
warnUnknownProperty(name);
},
onMountDOMComponent(debugID, element) {
cachedSource = element ? element._source : null;
onBeforeMountComponent(debugID, element) {
// TODO: This currently assumes that properties are all set before recursing
// and mounting children, which needn't be the case in the future.
lastDebugID = debugID;
},
onUpdateDOMComponent(debugID, element) {
cachedSource = element ? element._source : null;
onBeforeUpdateComponent(debugID, element) {
lastDebugID = debugID;
},
};

Expand Down
8 changes: 8 additions & 0 deletions src/renderers/shared/stack/reconciler/ReactReconciler.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,10 @@ var ReactReconciler = {
internalInstance._debugID,
'mountComponent'
);
ReactInstrumentation.debugTool.onBeforeMountComponent(
internalInstance._debugID,
internalInstance._currentElement
);
}
}
var markup = internalInstance.mountComponent(
Expand Down Expand Up @@ -150,6 +154,10 @@ var ReactReconciler = {
internalInstance._debugID,
'receiveComponent'
);
ReactInstrumentation.debugTool.onBeforeUpdateComponent(
internalInstance._debugID,
internalInstance._currentElement
);
}
}

Expand Down