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

Author: aweary

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(
       <div onClick="not a function" />,
     );
     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', () => {

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);
         }

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 <div onClick="woops" />;
+        }
+      }
+      ReactDOM.render(<Example />, 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');

src/renderers/dom/shared/__tests__/ReactDOMComponent-test.js

@@ -1690,8 +1690,8 @@ describe('ReactDOMComponent', () => {
       ReactTestUtils.renderIntoDocument(
         <div className="foo1">
           <div class="foo2" />
-          <div onClick="foo3" />
-          <div onclick="foo4" />
+          <div onClick={() => {}} />
+          <div onclick={() => {}} />
           <div className="foo5" />
           <div className="foo6" />
         </div>,

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,
     );