-
Notifications
You must be signed in to change notification settings - Fork 46.8k
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
Changes from 9 commits
408d2e6
ad3138b
eb8d72c
40f3c08
c377e35
9e5b795
a9fcfb3
987857f
3f82a3d
0e296e9
7f4736d
e016232
38f310c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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'); | ||
var node = ReactTestUtils.renderIntoDocument( | ||
<div onClick="not a function" />, | ||
); | ||
|
@@ -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); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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', () => { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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'); | ||
|
@@ -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', | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. | ||
|
@@ -227,6 +238,9 @@ function setInitialDOMProperties( | |
// Noop | ||
} else if (registrationNameModules.hasOwnProperty(propKey)) { | ||
if (nextProp) { | ||
if (__DEV__ && typeof nextProp !== 'function') { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You can move this There was a problem hiding this comment. Choose a reason for hiding this commentThe 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). There was a problem hiding this comment. Choose a reason for hiding this commentThe 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); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) { | ||
|
@@ -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) { | ||
|
@@ -980,6 +997,9 @@ var ReactDOMFiberComponent = { | |
} | ||
} | ||
} else if (registrationNameModules.hasOwnProperty(propKey)) { | ||
if (__DEV__ && typeof nextProp !== 'function') { | ||
warnForInvalidEventListener(propKey, nextProp); | ||
} | ||
if (nextProp) { | ||
ensureListeningTo(rootContainerElement, propKey); | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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'); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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'); | ||
|
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in c377e35