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

Jump to bottom

Use devtool for unknown property warning #5590

Merged
merged 3 commits into from
Dec 24, 2015
Merged

Use devtool for unknown property warning #5590

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a55ab28
Initial outline for new devtools api
jimfb Nov 12, 2015
30ef056
Moved unknown-prop warning into a devtool
jimfb Dec 3, 2015
26f3785
Use duck typing instead of allocating event objects
jimfb Dec 17, 2015
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
70 changes: 10 additions & 60 deletions src/renderers/dom/shared/DOMPropertyOperations.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@
'use strict';

var DOMProperty = require('DOMProperty');
var EventPluginRegistry = require('EventPluginRegistry');
var ReactDOMInstrumentation = require('ReactDOMInstrumentation');
var ReactPerf = require('ReactPerf');

var quoteAttributeValueForBrowser = require('quoteAttributeValueForBrowser');
Expand Down Expand Up @@ -51,59 +51,6 @@ function shouldIgnoreValue(propertyInfo, value) {
(propertyInfo.hasOverloadedBooleanValue && value === false);
}

if (__DEV__) {
var reactProps = {
children: true,
dangerouslySetInnerHTML: true,
key: true,
ref: true,
};
var warnedProperties = {};

var warnUnknownProperty = function(name) {
if (reactProps.hasOwnProperty(name) && reactProps[name] ||
warnedProperties.hasOwnProperty(name) && warnedProperties[name]) {
return;
}

warnedProperties[name] = true;
var lowerCasedName = name.toLowerCase();

// data-* attributes should be lowercase; suggest the lowercase version
var standardName = (
DOMProperty.isCustomAttribute(lowerCasedName) ?
lowerCasedName :
DOMProperty.getPossibleStandardName.hasOwnProperty(lowerCasedName) ?
DOMProperty.getPossibleStandardName[lowerCasedName] :
null
);

// 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?',
name,
standardName
);

var registrationName = (
EventPluginRegistry.possibleRegistrationNames.hasOwnProperty(
lowerCasedName
) ?
EventPluginRegistry.possibleRegistrationNames[lowerCasedName] :
null
);

warning(
registrationName == null,
'Unknown event handler property %s. Did you mean `%s`?',
name,
registrationName
);
};
}

/**
* Operations for dealing with DOM properties.
*/
Expand Down Expand Up @@ -140,6 +87,9 @@ var DOMPropertyOperations = {
* @return {?string} Markup string, or null if the property was invalid.
*/
createMarkupForProperty: function(name, value) {
if (__DEV__) {
ReactDOMInstrumentation.debugTool.onCreateMarkupForProperty(name, value);
}
var propertyInfo = DOMProperty.properties.hasOwnProperty(name) ?
DOMProperty.properties[name] : null;
if (propertyInfo) {
Expand All @@ -157,8 +107,6 @@ var DOMPropertyOperations = {
return '';
}
return name + '=' + quoteAttributeValueForBrowser(value);
} else if (__DEV__) {
warnUnknownProperty(name);
}
return null;
},
Expand All @@ -185,6 +133,9 @@ var DOMPropertyOperations = {
* @param {*} value
*/
setValueForProperty: function(node, name, value) {
if (__DEV__) {
ReactDOMInstrumentation.debugTool.onSetValueForProperty(node, name, value);
}
var propertyInfo = DOMProperty.properties.hasOwnProperty(name) ?
DOMProperty.properties[name] : null;
if (propertyInfo) {
Expand Down Expand Up @@ -219,8 +170,6 @@ var DOMPropertyOperations = {
}
} else if (DOMProperty.isCustomAttribute(name)) {
DOMPropertyOperations.setValueForAttribute(node, name, value);
} else if (__DEV__) {
warnUnknownProperty(name);
}
},

Expand All @@ -242,6 +191,9 @@ var DOMPropertyOperations = {
* @param {string} name
*/
deleteValueForProperty: function(node, name) {
if (__DEV__) {
ReactDOMInstrumentation.debugTool.onDeleteValueForProperty(node, name);
}
var propertyInfo = DOMProperty.properties.hasOwnProperty(name) ?
DOMProperty.properties[name] : null;
if (propertyInfo) {
Expand All @@ -263,8 +215,6 @@ var DOMPropertyOperations = {
}
} else if (DOMProperty.isCustomAttribute(name)) {
node.removeAttribute(name);
} else if (__DEV__) {
warnUnknownProperty(name);
}
},

Expand Down
66 changes: 66 additions & 0 deletions src/renderers/dom/shared/ReactDOMDebugTool.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,66 @@
/**
* Copyright 2013-2015, 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 ReactDOMDebugTool
*/

'use strict';

var ReactDOMUnknownPropertyDevtool = require('ReactDOMUnknownPropertyDevtool');

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 ReactDOMDebugTool = {
addDevtool(devtool) {
eventHandlers.push(devtool);
},
removeDevtool(devtool) {
for (var i = 0; i < eventHandlers.length; i++) {
if (eventHandlers[i] === devtool) {
eventHandlers.splice(i, 1);
i--;
}
}
},
onCreateMarkupForProperty(name, value) {
emitEvent('onCreateMarkupForProperty', name, value);
},
onSetValueForProperty(node, name, value) {
emitEvent('onSetValueForProperty', node, name, value);
},
onDeleteValueForProperty(node, name) {
emitEvent('onDeleteValueForProperty', node, name);
},
};

ReactDOMDebugTool.addDevtool(ReactDOMUnknownPropertyDevtool);

module.exports = ReactDOMDebugTool;
16 changes: 16 additions & 0 deletions src/renderers/dom/shared/ReactDOMInstrumentation.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
/**
* Copyright 2013-2015, 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 ReactDOMInstrumentation
*/

'use strict';

var ReactDOMDebugTool = require('ReactDOMDebugTool');

module.exports = {debugTool: ReactDOMDebugTool};
Copy link
Collaborator

Choose a reason for hiding this comment

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

What is the difference between “debug tool” and a “dev tool”? I got tripped by this a couple of times because I write a Devtool but I’m then adding it to a DebugTool which I refer to as ReactDOMInstrumentation.debugTool. Why is there more than one layer of indirection?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is only ever one debug tool. A debug tool doesn't (necessarily) have the overhead of emitting events, so it is suitable for collecting ultra-accurate performance metrics.

The default debug tool is an event emitter, into which you can plug dev tools.

A dev tool is something that is not performance critical. Things like emitting warnings, high-level performance metrics, interactive chrome extensions, etc. You want to maintain good performance, but you're willing to eat the cost of event delegation for the purposes of a slightly better API (ie. the ability to have multiple devtools at the same time, functions you don't implement are ignored, errors thrown in a devtool don't take down the app, etc).

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thank you for explaining. I’m curious because I wonder how something like Chrome extension would work once we port it to use this API. Currently Chrome extension works with production builds. Would that no longer be the case (presumably because production builds would use a noop debug tool)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Correct, my guess is that devtools would only work in dev mode, but it's an easy thing to change if we change our minds.

Copy link
Collaborator

Choose a reason for hiding this comment

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

To add more context, I am adding some events that will be useful both to React DevTools and new ReactPerf in #6068. If those are DEV-only, the new ReactPerf would also be DEV-only which would defeat the purpose of ReactPerf. How would debug tool / devtool distinction handle this problem?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think Sebastian was thinking about creating a third build called "profile", which follows all the "production" codepaths with the exception of having debugtool enabled.

Copy link
Contributor

Choose a reason for hiding this comment

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

Do these different builds have any overlap with the feature flags such as https://github.com/facebook/react/blob/3b96650e39ddda5ba49245713ef16dbc52d25e9e/src/shared/utils/ReactFeatureFlags.js?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, eventually. Right now it's controlled by __DEV__, and is not as simple as just replacing all __DEV__s with a feature flag for reasons around dead code elimination.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks, @jimfb. The “eventually” part is all I need to know :)

35 changes: 0 additions & 35 deletions src/renderers/dom/shared/__tests__/DOMPropertyOperations-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,41 +50,6 @@ describe('DOMPropertyOperations', function() {
)).toBe('id="simple"');
});

it('should warn about incorrect casing on properties', function() {
spyOn(console, 'error');
expect(DOMPropertyOperations.createMarkupForProperty(
'tabindex',
'1'
)).toBe(null);
expect(console.error.argsForCall.length).toBe(1);
expect(console.error.argsForCall[0][0]).toContain('tabIndex');
});

it('should warn about incorrect casing on event handlers', function() {
spyOn(console, 'error');
expect(DOMPropertyOperations.createMarkupForProperty(
'onclick',
'1'
)).toBe(null);
expect(DOMPropertyOperations.createMarkupForProperty(
'onKeydown',
'1'
)).toBe(null);
expect(console.error.argsForCall.length).toBe(2);
expect(console.error.argsForCall[0][0]).toContain('onClick');
expect(console.error.argsForCall[1][0]).toContain('onKeyDown');
});

it('should warn about class', function() {
spyOn(console, 'error');
expect(DOMPropertyOperations.createMarkupForProperty(
'class',
'muffins'
)).toBe(null);
expect(console.error.argsForCall.length).toBe(1);
expect(console.error.argsForCall[0][0]).toContain('className');
});

it('should create markup for boolean properties', function() {
expect(DOMPropertyOperations.createMarkupForProperty(
'checked',
Expand Down
23 changes: 23 additions & 0 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 @@ -1087,5 +1087,28 @@ describe('ReactDOMComponent', function() {
'See Link > a > ... > Link > a.'
);
});

it('should warn about incorrect casing on properties', function() {
spyOn(console, 'error');
ReactDOMServer.renderToString(React.createElement('input', {type: 'text', tabindex: '1'}));
expect(console.error.argsForCall.length).toBe(1);
expect(console.error.argsForCall[0][0]).toContain('tabIndex');
});

it('should warn about incorrect casing on event handlers', function() {
spyOn(console, 'error');
ReactDOMServer.renderToString(React.createElement('input', {type: 'text', onclick: '1'}));
ReactDOMServer.renderToString(React.createElement('input', {type: 'text', onKeydown: '1'}));
expect(console.error.argsForCall.length).toBe(2);
expect(console.error.argsForCall[0][0]).toContain('onClick');
expect(console.error.argsForCall[1][0]).toContain('onKeyDown');
});

it('should warn about class', function() {
spyOn(console, 'error');
ReactDOMServer.renderToString(React.createElement('div', {class: 'muffins'}));
expect(console.error.argsForCall.length).toBe(1);
expect(console.error.argsForCall[0][0]).toContain('className');
});
});
});
87 changes: 87 additions & 0 deletions src/renderers/dom/shared/devtools/ReactDOMUnknownPropertyDevtool.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,87 @@
/**
* Copyright 2013-2015, 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 ReactDOMUnknownPropertyDevtool
*/

'use strict';

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

var warning = require('warning');

if (__DEV__) {
var reactProps = {
children: true,
dangerouslySetInnerHTML: true,
key: true,
ref: true,
};
var warnedProperties = {};

var warnUnknownProperty = function(name) {
if (DOMProperty.properties.hasOwnProperty(name) || DOMProperty.isCustomAttribute(name)) {
return;
}
if (reactProps.hasOwnProperty(name) && reactProps[name] ||
warnedProperties.hasOwnProperty(name) && warnedProperties[name]) {
return;
}

warnedProperties[name] = true;
var lowerCasedName = name.toLowerCase();

// data-* attributes should be lowercase; suggest the lowercase version
var standardName = (
DOMProperty.isCustomAttribute(lowerCasedName) ?
lowerCasedName :
DOMProperty.getPossibleStandardName.hasOwnProperty(lowerCasedName) ?
DOMProperty.getPossibleStandardName[lowerCasedName] :
null
);

// 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?',
name,
standardName
);

var registrationName = (
EventPluginRegistry.possibleRegistrationNames.hasOwnProperty(
lowerCasedName
) ?
EventPluginRegistry.possibleRegistrationNames[lowerCasedName] :
null
);

warning(
registrationName == null,
'Unknown event handler property %s. Did you mean `%s`?',
name,
registrationName
);
};
}

var ReactDOMUnknownPropertyDevtool = {
onCreateMarkupForProperty(name, value) {
warnUnknownProperty(name);
},
onSetValueForProperty(node, name, value) {
warnUnknownProperty(name);
},
onDeleteValueForProperty(node, name) {
warnUnknownProperty(name);
},
}

module.exports = ReactDOMUnknownPropertyDevtool;