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
12 changes: 12 additions & 0 deletions src/renderers/__tests__/EventPluginHub-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -16,14 +16,18 @@ 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', () => {
// ReactDOMFiberComponent warns for non-function event listeners
spyOn(console, 'error');
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should still add an expectation. So that it doesn't emit some other unwanted warning by mistake in the future.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in c377e35

var node = ReactTestUtils.renderIntoDocument(
<div onClick="not a function" />,
);
Expand All @@ -32,6 +36,14 @@ describe('EventPluginHub', () => {
}).toThrowError(
'Expected onClick listener to be a function, instead got type string',
);
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 type string',
);
} else {
expectDev(console.error.calls.count()).toBe(0);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This warning isn't included in stack since it fails early

}
});

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 type %s%s',
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: I think we usually end warnings with periods (before the stack) so that they look like sentences.

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);
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is a very hot path. How do you feel about hoisting the condition right here (duplicating it) and only calling the function in the bad case?

}
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.only('should warn for non-functional event listeners', () => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Need to fix this, otherwise should be good to go

Copy link
Collaborator

Choose a reason for hiding this comment

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

-.only

spyOn(console, 'error');
Copy link
Collaborator

Choose a reason for hiding this comment

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

Only

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 type string\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