From a55ab282fa9f93e05ad9179f5b379013805db7b2 Mon Sep 17 00:00:00 2001 From: Jim Date: Thu, 12 Nov 2015 13:40:25 -0800 Subject: [PATCH 1/3] Initial outline for new devtools api --- src/renderers/dom/ReactDOM.js | 1 + src/renderers/dom/shared/ReactDOMDevtools.js | 52 ++++++++++++++++++++ 2 files changed, 53 insertions(+) create mode 100644 src/renderers/dom/shared/ReactDOMDevtools.js diff --git a/src/renderers/dom/ReactDOM.js b/src/renderers/dom/ReactDOM.js index c626433d3c90c..23ee4e2acfb44 100644 --- a/src/renderers/dom/ReactDOM.js +++ b/src/renderers/dom/ReactDOM.js @@ -14,6 +14,7 @@ 'use strict'; var ReactDOMComponentTree = require('ReactDOMComponentTree'); +var ReactDOMDevtools = require('ReactDOMDevtools'); var ReactDefaultInjection = require('ReactDefaultInjection'); var ReactMount = require('ReactMount'); var ReactPerf = require('ReactPerf'); diff --git a/src/renderers/dom/shared/ReactDOMDevtools.js b/src/renderers/dom/shared/ReactDOMDevtools.js new file mode 100644 index 0000000000000..d02e1a53959c3 --- /dev/null +++ b/src/renderers/dom/shared/ReactDOMDevtools.js @@ -0,0 +1,52 @@ +/** + * 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 ReactDOMDevtools + */ + +'use strict'; + +var warning = require('warning'); + +var eventHandlers = []; +var handlerDoesThrowForEvent = {}; + +var ReactDOMDevtools = { + addDevtool(devtool) { + eventHandlers.push(devtool); + }, + + removeDevtool(devtool) { + for (var i = 0; i < eventHandlers.length; i++) { + if (eventHandlers[i] === devtool) { + eventHandlers.splice(i, 1); + i--; + } + } + }, + + emitEvent(eventName, eventData) { + if (__DEV__) { + eventHandlers.forEach(function(handler) { + try { + handler.handleEvent(eventName, eventData); + } catch (e) { + warning( + !handlerDoesThrowForEvent[eventName], + 'exception thrown by devtool while handling %s: %s', + eventName, + e.message + ); + handlerDoesThrowForEvent[eventName] = true; + } + }); + } + }, +}; + +module.exports = ReactDOMDevtools; From 30ef056731f10a094661b0d288c68fbe0c044161 Mon Sep 17 00:00:00 2001 From: Jim Date: Thu, 3 Dec 2015 07:02:55 -0800 Subject: [PATCH 2/3] Moved unknown-prop warning into a devtool --- src/renderers/dom/ReactDOM.js | 3 + .../dom/shared/DOMPropertyOperations.js | 64 +------------- .../__tests__/DOMPropertyOperations-test.js | 35 -------- .../__tests__/ReactDOMComponent-test.js | 23 +++++ .../ReactDOMUnknownPropertyDevtool.js | 88 +++++++++++++++++++ 5 files changed, 118 insertions(+), 95 deletions(-) create mode 100644 src/renderers/dom/shared/devtools/ReactDOMUnknownPropertyDevtool.js diff --git a/src/renderers/dom/ReactDOM.js b/src/renderers/dom/ReactDOM.js index 23ee4e2acfb44..56e8bad6d24fe 100644 --- a/src/renderers/dom/ReactDOM.js +++ b/src/renderers/dom/ReactDOM.js @@ -15,6 +15,7 @@ var ReactDOMComponentTree = require('ReactDOMComponentTree'); var ReactDOMDevtools = require('ReactDOMDevtools'); +var ReactDOMUnknownPropertyDevtool = require('ReactDOMUnknownPropertyDevtool'); var ReactDefaultInjection = require('ReactDefaultInjection'); var ReactMount = require('ReactMount'); var ReactPerf = require('ReactPerf'); @@ -136,6 +137,8 @@ if (__DEV__) { break; } } + + ReactDOMDevtools.addDevtool(new ReactDOMUnknownPropertyDevtool()); } } diff --git a/src/renderers/dom/shared/DOMPropertyOperations.js b/src/renderers/dom/shared/DOMPropertyOperations.js index 94b47f1396c63..eda5f3979de98 100644 --- a/src/renderers/dom/shared/DOMPropertyOperations.js +++ b/src/renderers/dom/shared/DOMPropertyOperations.js @@ -12,7 +12,7 @@ 'use strict'; var DOMProperty = require('DOMProperty'); -var EventPluginRegistry = require('EventPluginRegistry'); +var ReactDOMDevtools = require('ReactDOMDevtools'); var ReactPerf = require('ReactPerf'); var quoteAttributeValueForBrowser = require('quoteAttributeValueForBrowser'); @@ -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. */ @@ -140,6 +87,7 @@ var DOMPropertyOperations = { * @return {?string} Markup string, or null if the property was invalid. */ createMarkupForProperty: function(name, value) { + ReactDOMDevtools.emitEvent('createMarkupForProperty', {name, value}); var propertyInfo = DOMProperty.properties.hasOwnProperty(name) ? DOMProperty.properties[name] : null; if (propertyInfo) { @@ -157,8 +105,6 @@ var DOMPropertyOperations = { return ''; } return name + '=' + quoteAttributeValueForBrowser(value); - } else if (__DEV__) { - warnUnknownProperty(name); } return null; }, @@ -185,6 +131,7 @@ var DOMPropertyOperations = { * @param {*} value */ setValueForProperty: function(node, name, value) { + ReactDOMDevtools.emitEvent('setValueForProperty', {node, name, value}); var propertyInfo = DOMProperty.properties.hasOwnProperty(name) ? DOMProperty.properties[name] : null; if (propertyInfo) { @@ -219,8 +166,6 @@ var DOMPropertyOperations = { } } else if (DOMProperty.isCustomAttribute(name)) { DOMPropertyOperations.setValueForAttribute(node, name, value); - } else if (__DEV__) { - warnUnknownProperty(name); } }, @@ -242,6 +187,7 @@ var DOMPropertyOperations = { * @param {string} name */ deleteValueForProperty: function(node, name) { + ReactDOMDevtools.emitEvent('deleteValueForProperty', {node, name}); var propertyInfo = DOMProperty.properties.hasOwnProperty(name) ? DOMProperty.properties[name] : null; if (propertyInfo) { @@ -263,8 +209,6 @@ var DOMPropertyOperations = { } } else if (DOMProperty.isCustomAttribute(name)) { node.removeAttribute(name); - } else if (__DEV__) { - warnUnknownProperty(name); } }, diff --git a/src/renderers/dom/shared/__tests__/DOMPropertyOperations-test.js b/src/renderers/dom/shared/__tests__/DOMPropertyOperations-test.js index 099fd55da7996..4dc95debae356 100644 --- a/src/renderers/dom/shared/__tests__/DOMPropertyOperations-test.js +++ b/src/renderers/dom/shared/__tests__/DOMPropertyOperations-test.js @@ -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', diff --git a/src/renderers/dom/shared/__tests__/ReactDOMComponent-test.js b/src/renderers/dom/shared/__tests__/ReactDOMComponent-test.js index 950a2f4798d3d..1e920255932b0 100644 --- a/src/renderers/dom/shared/__tests__/ReactDOMComponent-test.js +++ b/src/renderers/dom/shared/__tests__/ReactDOMComponent-test.js @@ -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'); + }); }); }); diff --git a/src/renderers/dom/shared/devtools/ReactDOMUnknownPropertyDevtool.js b/src/renderers/dom/shared/devtools/ReactDOMUnknownPropertyDevtool.js new file mode 100644 index 0000000000000..8e7779dea4dce --- /dev/null +++ b/src/renderers/dom/shared/devtools/ReactDOMUnknownPropertyDevtool.js @@ -0,0 +1,88 @@ +/** + * 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 (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 + ); + }; +} + +class ReactDOMUnknownPropertyDevtool { + handleEvent(eventName, eventData) { + switch (eventName) { + case 'createMarkupForProperty': + case 'setValueForProperty': + case 'deleteValueForProperty': + var name = eventData.name; + if (!DOMProperty.properties.hasOwnProperty(name) && + !DOMProperty.isCustomAttribute(name)) { + warnUnknownProperty(name); + } + break; + } + } +} + +module.exports = ReactDOMUnknownPropertyDevtool; From 26f3785a8c2e78ca8d393eb153f1b879d318b858 Mon Sep 17 00:00:00 2001 From: jim Date: Wed, 16 Dec 2015 19:13:49 -0800 Subject: [PATCH 3/3] Use duck typing instead of allocating event objects --- src/renderers/dom/ReactDOM.js | 4 -- .../dom/shared/DOMPropertyOperations.js | 14 ++-- src/renderers/dom/shared/ReactDOMDebugTool.js | 66 +++++++++++++++++++ src/renderers/dom/shared/ReactDOMDevtools.js | 52 --------------- .../dom/shared/ReactDOMInstrumentation.js | 16 +++++ .../ReactDOMUnknownPropertyDevtool.js | 27 ++++---- 6 files changed, 105 insertions(+), 74 deletions(-) create mode 100644 src/renderers/dom/shared/ReactDOMDebugTool.js delete mode 100644 src/renderers/dom/shared/ReactDOMDevtools.js create mode 100644 src/renderers/dom/shared/ReactDOMInstrumentation.js diff --git a/src/renderers/dom/ReactDOM.js b/src/renderers/dom/ReactDOM.js index 56e8bad6d24fe..c626433d3c90c 100644 --- a/src/renderers/dom/ReactDOM.js +++ b/src/renderers/dom/ReactDOM.js @@ -14,8 +14,6 @@ 'use strict'; var ReactDOMComponentTree = require('ReactDOMComponentTree'); -var ReactDOMDevtools = require('ReactDOMDevtools'); -var ReactDOMUnknownPropertyDevtool = require('ReactDOMUnknownPropertyDevtool'); var ReactDefaultInjection = require('ReactDefaultInjection'); var ReactMount = require('ReactMount'); var ReactPerf = require('ReactPerf'); @@ -137,8 +135,6 @@ if (__DEV__) { break; } } - - ReactDOMDevtools.addDevtool(new ReactDOMUnknownPropertyDevtool()); } } diff --git a/src/renderers/dom/shared/DOMPropertyOperations.js b/src/renderers/dom/shared/DOMPropertyOperations.js index eda5f3979de98..25598a8f50446 100644 --- a/src/renderers/dom/shared/DOMPropertyOperations.js +++ b/src/renderers/dom/shared/DOMPropertyOperations.js @@ -12,7 +12,7 @@ 'use strict'; var DOMProperty = require('DOMProperty'); -var ReactDOMDevtools = require('ReactDOMDevtools'); +var ReactDOMInstrumentation = require('ReactDOMInstrumentation'); var ReactPerf = require('ReactPerf'); var quoteAttributeValueForBrowser = require('quoteAttributeValueForBrowser'); @@ -87,7 +87,9 @@ var DOMPropertyOperations = { * @return {?string} Markup string, or null if the property was invalid. */ createMarkupForProperty: function(name, value) { - ReactDOMDevtools.emitEvent('createMarkupForProperty', {name, value}); + if (__DEV__) { + ReactDOMInstrumentation.debugTool.onCreateMarkupForProperty(name, value); + } var propertyInfo = DOMProperty.properties.hasOwnProperty(name) ? DOMProperty.properties[name] : null; if (propertyInfo) { @@ -131,7 +133,9 @@ var DOMPropertyOperations = { * @param {*} value */ setValueForProperty: function(node, name, value) { - ReactDOMDevtools.emitEvent('setValueForProperty', {node, name, value}); + if (__DEV__) { + ReactDOMInstrumentation.debugTool.onSetValueForProperty(node, name, value); + } var propertyInfo = DOMProperty.properties.hasOwnProperty(name) ? DOMProperty.properties[name] : null; if (propertyInfo) { @@ -187,7 +191,9 @@ var DOMPropertyOperations = { * @param {string} name */ deleteValueForProperty: function(node, name) { - ReactDOMDevtools.emitEvent('deleteValueForProperty', {node, name}); + if (__DEV__) { + ReactDOMInstrumentation.debugTool.onDeleteValueForProperty(node, name); + } var propertyInfo = DOMProperty.properties.hasOwnProperty(name) ? DOMProperty.properties[name] : null; if (propertyInfo) { diff --git a/src/renderers/dom/shared/ReactDOMDebugTool.js b/src/renderers/dom/shared/ReactDOMDebugTool.js new file mode 100644 index 0000000000000..5dbd0c0ea4b5f --- /dev/null +++ b/src/renderers/dom/shared/ReactDOMDebugTool.js @@ -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; diff --git a/src/renderers/dom/shared/ReactDOMDevtools.js b/src/renderers/dom/shared/ReactDOMDevtools.js deleted file mode 100644 index d02e1a53959c3..0000000000000 --- a/src/renderers/dom/shared/ReactDOMDevtools.js +++ /dev/null @@ -1,52 +0,0 @@ -/** - * 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 ReactDOMDevtools - */ - -'use strict'; - -var warning = require('warning'); - -var eventHandlers = []; -var handlerDoesThrowForEvent = {}; - -var ReactDOMDevtools = { - addDevtool(devtool) { - eventHandlers.push(devtool); - }, - - removeDevtool(devtool) { - for (var i = 0; i < eventHandlers.length; i++) { - if (eventHandlers[i] === devtool) { - eventHandlers.splice(i, 1); - i--; - } - } - }, - - emitEvent(eventName, eventData) { - if (__DEV__) { - eventHandlers.forEach(function(handler) { - try { - handler.handleEvent(eventName, eventData); - } catch (e) { - warning( - !handlerDoesThrowForEvent[eventName], - 'exception thrown by devtool while handling %s: %s', - eventName, - e.message - ); - handlerDoesThrowForEvent[eventName] = true; - } - }); - } - }, -}; - -module.exports = ReactDOMDevtools; diff --git a/src/renderers/dom/shared/ReactDOMInstrumentation.js b/src/renderers/dom/shared/ReactDOMInstrumentation.js new file mode 100644 index 0000000000000..11785df4ad33f --- /dev/null +++ b/src/renderers/dom/shared/ReactDOMInstrumentation.js @@ -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}; diff --git a/src/renderers/dom/shared/devtools/ReactDOMUnknownPropertyDevtool.js b/src/renderers/dom/shared/devtools/ReactDOMUnknownPropertyDevtool.js index 8e7779dea4dce..64c1c19582c40 100644 --- a/src/renderers/dom/shared/devtools/ReactDOMUnknownPropertyDevtool.js +++ b/src/renderers/dom/shared/devtools/ReactDOMUnknownPropertyDevtool.js @@ -26,6 +26,9 @@ if (__DEV__) { 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; @@ -69,20 +72,16 @@ if (__DEV__) { }; } -class ReactDOMUnknownPropertyDevtool { - handleEvent(eventName, eventData) { - switch (eventName) { - case 'createMarkupForProperty': - case 'setValueForProperty': - case 'deleteValueForProperty': - var name = eventData.name; - if (!DOMProperty.properties.hasOwnProperty(name) && - !DOMProperty.isCustomAttribute(name)) { - warnUnknownProperty(name); - } - break; - } - } +var ReactDOMUnknownPropertyDevtool = { + onCreateMarkupForProperty(name, value) { + warnUnknownProperty(name); + }, + onSetValueForProperty(node, name, value) { + warnUnknownProperty(name); + }, + onDeleteValueForProperty(node, name) { + warnUnknownProperty(name); + }, } module.exports = ReactDOMUnknownPropertyDevtool;