Skip to content

Commit

Permalink
Fix a warning issue uncovered by flat bundle testing
Browse files Browse the repository at this point in the history 
With flat bundles, we couldn't produce a good warning for <div onclick={}> 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).
  • Loading branch information
@gaearon
gaearon committed Nov 23, 2017
1 parent 4da70e7 commit e143a76
Show file tree
Hide file tree
Showing 4 changed files with 81 additions and 57 deletions.
33 changes: 24 additions & 9 deletions packages/react-dom/src/__tests__/ReactDOMComponent-test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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).
}
});

Expand All @@ -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');
}
});
Expand Down Expand Up @@ -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 **)',
);
}
});
Expand Down
2 changes: 1 addition & 1 deletion packages/react-dom/src/client/ReactDOMFiberComponent.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
2 changes: 1 addition & 1 deletion packages/react-dom/src/server/ReactPartialRenderer.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
101 changes: 55 additions & 46 deletions packages/react-dom/src/shared/ReactDOMUnknownPropertyHook.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@

import {
registrationNameModules,
plugins,
possibleRegistrationNames,
} from 'events/EventPluginRegistry';
import {ReactDebugCurrentFrame} from 'shared/ReactGlobalSharedState';
Expand All @@ -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[a-zA-Z]/;
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 (lowerCasedName.indexOf('on') === 0 && lowerCasedName.length > 2) {
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;
}
Expand All @@ -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,
Expand Down Expand Up @@ -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);
}
Expand Down Expand Up @@ -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);
}

0 comments on commit e143a76

Please sign in to comment.