Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Warn early for non-functional event listeners #10453

Merged
merged 13 commits into from
Aug 23, 2017
13 changes: 12 additions & 1 deletion src/renderers/__tests__/EventPluginHub-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -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', () => {
Expand Down
20 changes: 20 additions & 0 deletions src/renderers/dom/fiber/ReactDOMFiberComponent.js
Original file line number Diff line number Diff line change
Expand Up @@ -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');
Expand Down Expand Up @@ -122,6 +123,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.
Expand Down Expand Up @@ -227,6 +238,9 @@ function setInitialDOMProperties(
// Noop
} else if (registrationNameModules.hasOwnProperty(propKey)) {
if (nextProp) {
if (__DEV__ && typeof nextProp !== 'function') {
Copy link
Collaborator

@sebmarkbage sebmarkbage Aug 14, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can move this typeof nextProp !== 'function' assertion into the warnForInvalidEventListener function. E.g. as the first argument to warning(). Makes it easier to keep in sync and update. E.g. if we want to support other type of listeners like {handleEvent() { }}.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's how it used to do but I wanted to avoid extra overhead for calls for every single prop. These things are small but sometimes add up (like when we fixed DEV performance in 15.3).

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But I don't feel strongly. Didn't think about the object case where it could change.

warnForInvalidEventListener(propKey, nextProp);
}
ensureListeningTo(rootContainerElement, propKey);
}
} else if (isCustomComponentTag) {
Expand Down Expand Up @@ -740,6 +754,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) {
Expand Down Expand Up @@ -980,6 +997,9 @@ var ReactDOMFiberComponent = {
}
}
} else if (registrationNameModules.hasOwnProperty(propKey)) {
if (__DEV__ && typeof nextProp !== 'function') {
warnForInvalidEventListener(propKey, nextProp);
}
if (nextProp) {
ensureListeningTo(rootContainerElement, propKey);
}
Expand Down
22 changes: 22 additions & 0 deletions src/renderers/dom/fiber/__tests__/ReactDOMFiber-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -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');
Expand Down
4 changes: 2 additions & 2 deletions src/renderers/dom/shared/__tests__/ReactDOMComponent-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -1653,8 +1653,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>,
Expand Down
2 changes: 1 addition & 1 deletion src/renderers/shared/shared/event/EventPluginHub.js
Original file line number Diff line number Diff line change
Expand Up @@ -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,
);
Expand Down