Fix a warning issue uncovered by flat bundle testing

gaearon · gaearon · commit 8888c463c2f7 · 2017-11-23T17:33:55.000Z
With flat bundles, we couldn't produce a good warning for &lt;div onclick={}&gt; on SSR
because it doesn't use the event system. However the issue was not visible in normal
Jest runs because the event plugins have been injected by the time the test ran.

To solve this, I am explicitly passing whether event system is available as an argument
to the hook. This makes the behavior consistent between source and bundle tests. Then
I change the tests to document the actual logic and _attempt_ to show a nice message
(e.g. we know for sure `onclick` is a bad event but we don't know the right name for it
on the server so we just say a generic message about camelCase naming convention).
diff --git a/packages/react-dom/src/__tests__/ReactDOMComponent-test.js b/packages/react-dom/src/__tests__/ReactDOMComponent-test.js
@@ -1709,15 +1709,25 @@ describe('ReactDOMComponent', () => {
     it('should warn about incorrect casing on event handlers (ssr)', () => {
       spyOnDev(console, 'error');
       ReactDOMServer.renderToString(
-        React.createElement('input', {type: 'text', onclick: '1'}),
+        React.createElement('input', {type: 'text', oninput: '1'}),
       );
       ReactDOMServer.renderToString(
         React.createElement('input', {type: 'text', onKeydown: '1'}),
       );
       if (__DEV__) {
-        expect(console.error.calls.count()).toBe(2);
-        expect(console.error.calls.argsFor(0)[0]).toContain('onClick');
-        expect(console.error.calls.argsFor(1)[0]).toContain('onKeyDown');
+        expect(console.error.calls.count()).toBe(1);
+        expect(console.error.calls.argsFor(0)[0]).toContain(
+          'Invalid event handler property `oninput`. ' +
+            'React events use the camelCase naming convention, ' +
+            // Note: we don't know the right event name so we
+            // use a generic one (onClick) as a suggestion.
+            // This is because we don't bundle the event system
+            // on the server.
+            'for example `onClick`.',
+        );
+        // We can't warn for `onKeydown` on the server because
+        // there is no way tell if this is a valid event or not
+        // without access to the event system (which we don't bundle).
       }
     });
 
@@ -1735,14 +1745,14 @@ describe('ReactDOMComponent', () => {
     it('should warn about incorrect casing on event handlers', () => {
       spyOnDev(console, 'error');
       ReactTestUtils.renderIntoDocument(
-        React.createElement('input', {type: 'text', onclick: '1'}),
+        React.createElement('input', {type: 'text', oninput: '1'}),
       );
       ReactTestUtils.renderIntoDocument(
         React.createElement('input', {type: 'text', onKeydown: '1'}),
       );
       if (__DEV__) {
         expect(console.error.calls.count()).toBe(2);
-        expect(console.error.calls.argsFor(0)[0]).toContain('onClick');
+        expect(console.error.calls.argsFor(0)[0]).toContain('onInput');
         expect(console.error.calls.argsFor(1)[0]).toContain('onKeyDown');
       }
     });
@@ -1860,15 +1870,20 @@ describe('ReactDOMComponent', () => {
     it('gives source code refs for unknown prop warning (ssr)', () => {
       spyOnDev(console, 'error');
       ReactDOMServer.renderToString(<div class="paladin" />);
-      ReactDOMServer.renderToString(<input type="text" onclick="1" />);
+      ReactDOMServer.renderToString(<input type="text" oninput="1" />);
       if (__DEV__) {
         expect(console.error.calls.count()).toBe(2);
         expect(normalizeCodeLocInfo(console.error.calls.argsFor(0)[0])).toBe(
           'Warning: Invalid DOM property `class`. Did you mean `className`?\n    in div (at **)',
         );
         expect(normalizeCodeLocInfo(console.error.calls.argsFor(1)[0])).toBe(
-          'Warning: Invalid event handler property `onclick`. Did you mean ' +
-            '`onClick`?\n    in input (at **)',
+          'Warning: Invalid event handler property `oninput`. ' +
+            // Note: we don't know the right event name so we
+            // use a generic one (onClick) as a suggestion.
+            // This is because we don't bundle the event system
+            // on the server.
+            'React events use the camelCase naming convention, for example `onClick`.' +
+            '\n    in input (at **)',
         );
       }
     });
diff --git a/packages/react-dom/src/client/ReactDOMFiberComponent.js b/packages/react-dom/src/client/ReactDOMFiberComponent.js
@@ -67,7 +67,7 @@ if (__DEV__) {
   var validatePropertiesInDevelopment = function(type, props) {
     validateARIAProperties(type, props);
     validateInputProperties(type, props);
-    validateUnknownProperties(type, props);
+    validateUnknownProperties(type, props, /* canUseEventSystem */ true);
   };
 
   // HTML parsing normalizes CR and CRLF to LF.
diff --git a/packages/react-dom/src/server/ReactPartialRenderer.js b/packages/react-dom/src/server/ReactPartialRenderer.js
@@ -54,7 +54,7 @@ if (__DEV__) {
   var validatePropertiesInDevelopment = function(type, props) {
     validateARIAProperties(type, props);
     validateInputProperties(type, props);
-    validateUnknownProperties(type, props);
+    validateUnknownProperties(type, props, /* canUseEventSystem */ false);
   };
 
   var describeStackFrame = function(element): string {
diff --git a/packages/react-dom/src/shared/ReactDOMUnknownPropertyHook.js b/packages/react-dom/src/shared/ReactDOMUnknownPropertyHook.js
@@ -7,7 +7,6 @@
 
 import {
   registrationNameModules,
-  plugins,
   possibleRegistrationNames,
 } from 'events/EventPluginRegistry';
 import {ReactDebugCurrentFrame} from 'shared/ReactGlobalSharedState';
@@ -30,51 +29,72 @@ function getStackAddendum() {
 if (__DEV__) {
   var warnedProperties = {};
   var hasOwnProperty = Object.prototype.hasOwnProperty;
-  var EVENT_NAME_REGEX = /^on[A-Z]/;
+  var EVENT_NAME_REGEX = /^on./;
+  var INVALID_EVENT_NAME_REGEX = /^on[^A-Z]/;
   var rARIA = new RegExp('^(aria)-[' + ATTRIBUTE_NAME_CHAR + ']*$');
   var rARIACamel = new RegExp('^(aria)[A-Z][' + ATTRIBUTE_NAME_CHAR + ']*$');
 
-  var validateProperty = function(tagName, name, value) {
+  var validateProperty = function(tagName, name, value, canUseEventSystem) {
     if (hasOwnProperty.call(warnedProperties, name) && warnedProperties[name]) {
       return true;
     }
 
-    if (registrationNameModules.hasOwnProperty(name)) {
-      return true;
-    }
-
-    if (plugins.length === 0 && EVENT_NAME_REGEX.test(name)) {
-      // If no event plugins have been injected, we might be in a server environment.
-      // Don't check events in this case.
-      return true;
-    }
-
     var lowerCasedName = name.toLowerCase();
-    var registrationName = possibleRegistrationNames.hasOwnProperty(
-      lowerCasedName,
-    )
-      ? possibleRegistrationNames[lowerCasedName]
-      : null;
-
-    if (registrationName != null) {
+    if (lowerCasedName === 'onfocusin' || lowerCasedName === 'onfocusout') {
       warning(
         false,
-        'Invalid event handler property `%s`. Did you mean `%s`?%s',
-        name,
-        registrationName,
-        getStackAddendum(),
+        'React uses onFocus and onBlur instead of onFocusIn and onFocusOut. ' +
+          'All React events are normalized to bubble, so onFocusIn and onFocusOut ' +
+          'are not needed/supported by React.',
       );
       warnedProperties[name] = true;
       return true;
     }
 
-    if (lowerCasedName.indexOf('on') === 0 && lowerCasedName.length > 2) {
-      warning(
-        false,
-        'Unknown event handler property `%s`. It will be ignored.%s',
-        name,
-        getStackAddendum(),
-      );
+    // We can't rely on the event system being injected on the server.
+    if (canUseEventSystem) {
+      if (registrationNameModules.hasOwnProperty(name)) {
+        return true;
+      }
+      var registrationName = possibleRegistrationNames.hasOwnProperty(
+        lowerCasedName,
+      )
+        ? possibleRegistrationNames[lowerCasedName]
+        : null;
+      if (registrationName != null) {
+        warning(
+          false,
+          'Invalid event handler property `%s`. Did you mean `%s`?%s',
+          name,
+          registrationName,
+          getStackAddendum(),
+        );
+        warnedProperties[name] = true;
+        return true;
+      }
+      if (EVENT_NAME_REGEX.test(name)) {
+        warning(
+          false,
+          'Unknown event handler property `%s`. It will be ignored.%s',
+          name,
+          getStackAddendum(),
+        );
+        warnedProperties[name] = true;
+        return true;
+      }
+    } else if (EVENT_NAME_REGEX.test(name)) {
+      // If no event plugins have been injected, we are in a server environment.
+      // So we can't tell if the event name is correct for sure, but we can filter
+      // out known bad ones like `onclick`. We can't suggest a specific replacement though.
+      if (INVALID_EVENT_NAME_REGEX.test(name)) {
+        warning(
+          false,
+          'Invalid event handler property `%s`. ' +
+            'React events use the camelCase naming convention, for example `onClick`.%s',
+          name,
+          getStackAddendum(),
+        );
+      }
       warnedProperties[name] = true;
       return true;
     }
@@ -84,17 +104,6 @@ if (__DEV__) {
       return true;
     }
 
-    if (lowerCasedName === 'onfocusin' || lowerCasedName === 'onfocusout') {
-      warning(
-        false,
-        'React uses onFocus and onBlur instead of onFocusIn and onFocusOut. ' +
-          'All React events are normalized to bubble, so onFocusIn and onFocusOut ' +
-          'are not needed/supported by React.',
-      );
-      warnedProperties[name] = true;
-      return true;
-    }
-
     if (lowerCasedName === 'innerhtml') {
       warning(
         false,
@@ -233,10 +242,10 @@ if (__DEV__) {
   };
 }
 
-var warnUnknownProperties = function(type, props) {
+var warnUnknownProperties = function(type, props, canUseEventSystem) {
   var unknownProps = [];
   for (var key in props) {
-    var isValid = validateProperty(type, key, props[key]);
+    var isValid = validateProperty(type, key, props[key], canUseEventSystem);
     if (!isValid) {
       unknownProps.push(key);
     }
@@ -266,9 +275,9 @@ var warnUnknownProperties = function(type, props) {
   }
 };
 
-export function validateProperties(type, props) {
+export function validateProperties(type, props, canUseEventSystem) {
   if (isCustomComponent(type, props)) {
     return;
   }
-  warnUnknownProperties(type, props);
+  warnUnknownProperties(type, props, canUseEventSystem);
 }
