From 34780da5c8d702daf84a46ba436a3c580b95a3cb Mon Sep 17 00:00:00 2001 From: Brandon Dail Date: Wed, 23 Aug 2017 07:38:48 -0500 Subject: [PATCH] Warn early for non-functional event listeners (#10453) * Add early warning for non-functional event listeners * Use functional listeners in ReactDOMComponent test To avoid triggering the non-functional event listener component * spy on console.error in non-function EventPluginHub test This should warn from ReactDOMFiberComponent, but we just ignore it here * Remove redundant check for listener * Add expectation for non-functional listener warning in EventPluginHub * Hoist listener typeof check in hot paths * Include stack addendum in non-functional listener warning * Make it pretty * Remove it.onnly from ReactDOMFiber test * Fix message * Update expected message * Change invariant message to match the new style * Fix remaining warning --- .../__tests__/EventPluginHub-test.js | 13 ++++++++++- .../dom/fiber/ReactDOMFiberComponent.js | 20 +++++++++++++++++ .../dom/fiber/__tests__/ReactDOMFiber-test.js | 22 +++++++++++++++++++ .../__tests__/ReactDOMComponent-test.js | 4 ++-- .../shared/shared/event/EventPluginHub.js | 2 +- 5 files changed, 57 insertions(+), 4 deletions(-) diff --git a/src/renderers/__tests__/EventPluginHub-test.js b/src/renderers/__tests__/EventPluginHub-test.js index 03b33abe00..4ad5bd3db2 100644 --- a/src/renderers/__tests__/EventPluginHub-test.js +++ b/src/renderers/__tests__/EventPluginHub-test.js @@ -16,22 +16,33 @@ jest.mock('isEventSupported'); describe('EventPluginHub', () => { var React; var ReactTestUtils; + var ReactDOMFeatureFlags; beforeEach(() => { jest.resetModules(); React = require('react'); ReactTestUtils = require('react-dom/test-utils'); + ReactDOMFeatureFlags = require('ReactDOMFeatureFlags'); }); it('should prevent non-function listeners, at dispatch', () => { + spyOn(console, 'error'); var node = ReactTestUtils.renderIntoDocument(
, ); expect(function() { ReactTestUtils.SimulateNative.click(node); }).toThrowError( - 'Expected onClick listener to be a function, instead got type string', + 'Expected `onClick` listener to be a function, instead got a value of `string` type.', ); + if (ReactDOMFeatureFlags.useFiber) { + expectDev(console.error.calls.count()).toBe(1); + expectDev(console.error.calls.argsFor(0)[0]).toContain( + 'Expected `onClick` listener to be a function, instead got a value of `string` type.', + ); + } else { + expectDev(console.error.calls.count()).toBe(0); + } }); it('should not prevent null listeners, at dispatch', () => { diff --git a/src/renderers/dom/fiber/ReactDOMFiberComponent.js b/src/renderers/dom/fiber/ReactDOMFiberComponent.js index 085bbc5559..475960cdf7 100644 --- a/src/renderers/dom/fiber/ReactDOMFiberComponent.js +++ b/src/renderers/dom/fiber/ReactDOMFiberComponent.js @@ -34,6 +34,7 @@ var setTextContent = require('setTextContent'); if (__DEV__) { var warning = require('fbjs/lib/warning'); + var {getCurrentFiberStackAddendum} = require('ReactDebugCurrentFiber'); var ReactDOMInvalidARIAHook = require('ReactDOMInvalidARIAHook'); var ReactDOMNullInputValuePropHook = require('ReactDOMNullInputValuePropHook'); var ReactDOMUnknownPropertyHook = require('ReactDOMUnknownPropertyHook'); @@ -118,6 +119,16 @@ if (__DEV__) { warning(false, 'Extra attributes from the server: %s', names); }; + var warnForInvalidEventListener = function(registrationName, listener) { + warning( + false, + 'Expected `%s` listener to be a function, instead got a value of `%s` type.%s', + registrationName, + typeof listener, + getCurrentFiberStackAddendum(), + ); + }; + var testDocument; // Parse the HTML and read it back to normalize the HTML string so that it // can be used for comparison. @@ -223,6 +234,9 @@ function setInitialDOMProperties( // Noop } else if (registrationNameModules.hasOwnProperty(propKey)) { if (nextProp) { + if (__DEV__ && typeof nextProp !== 'function') { + warnForInvalidEventListener(propKey, nextProp); + } ensureListeningTo(rootContainerElement, propKey); } } else if (isCustomComponentTag) { @@ -695,6 +709,9 @@ var ReactDOMFiberComponent = { } else if (registrationNameModules.hasOwnProperty(propKey)) { if (nextProp) { // We eagerly listen to this even though we haven't committed yet. + if (__DEV__ && typeof nextProp !== 'function') { + warnForInvalidEventListener(propKey, nextProp); + } ensureListeningTo(rootContainerElement, propKey); } if (!updatePayload && lastProp !== nextProp) { @@ -934,6 +951,9 @@ var ReactDOMFiberComponent = { } } } else if (registrationNameModules.hasOwnProperty(propKey)) { + if (__DEV__ && typeof nextProp !== 'function') { + warnForInvalidEventListener(propKey, nextProp); + } if (nextProp) { ensureListeningTo(rootContainerElement, propKey); } diff --git a/src/renderers/dom/fiber/__tests__/ReactDOMFiber-test.js b/src/renderers/dom/fiber/__tests__/ReactDOMFiber-test.js index bde7371836..2910041d9b 100644 --- a/src/renderers/dom/fiber/__tests__/ReactDOMFiber-test.js +++ b/src/renderers/dom/fiber/__tests__/ReactDOMFiber-test.js @@ -18,6 +18,10 @@ var ReactTestUtils = require('react-dom/test-utils'); var PropTypes = require('prop-types'); describe('ReactDOMFiber', () => { + function normalizeCodeLocInfo(str) { + return str && str.replace(/\(at .+?:\d+\)/g, '(at **)'); + } + var container; var ReactFeatureFlags; @@ -913,6 +917,24 @@ describe('ReactDOMFiber', () => { ]); }); + it('should warn for non-functional event listeners', () => { + spyOn(console, 'error'); + class Example extends React.Component { + render() { + return
; + } + } + ReactDOM.render(, container); + expectDev(console.error.calls.count()).toBe(1); + expectDev( + normalizeCodeLocInfo(console.error.calls.argsFor(0)[0]), + ).toContain( + 'Expected `onClick` listener to be a function, instead got a value of `string` type.\n' + + ' in div (at **)\n' + + ' in Example (at **)', + ); + }); + it('should not update event handlers until commit', () => { let ops = []; const handlerA = () => ops.push('A'); diff --git a/src/renderers/dom/shared/__tests__/ReactDOMComponent-test.js b/src/renderers/dom/shared/__tests__/ReactDOMComponent-test.js index 835a45c608..981c9d3ed2 100644 --- a/src/renderers/dom/shared/__tests__/ReactDOMComponent-test.js +++ b/src/renderers/dom/shared/__tests__/ReactDOMComponent-test.js @@ -1690,8 +1690,8 @@ describe('ReactDOMComponent', () => { ReactTestUtils.renderIntoDocument(
-
-
+
{}} /> +
{}} />
, diff --git a/src/renderers/shared/shared/event/EventPluginHub.js b/src/renderers/shared/shared/event/EventPluginHub.js index 6d1bf6d790..20f34c8a8b 100644 --- a/src/renderers/shared/shared/event/EventPluginHub.js +++ b/src/renderers/shared/shared/event/EventPluginHub.js @@ -163,7 +163,7 @@ var EventPluginHub = { invariant( !listener || typeof listener === 'function', - 'Expected %s listener to be a function, instead got type %s', + 'Expected `%s` listener to be a function, instead got a value of `%s` type.', registrationName, typeof listener, );