-
Notifications
You must be signed in to change notification settings - Fork 49.8k
Warn early for non-functional event listeners #10453
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
Changes from 7 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); | ||
|
Contributor
Author
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', | ||
|
||
| 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') { | ||
|
Collaborator
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
Collaborator
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).
Collaborator
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); | ||
|
Collaborator
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,22 @@ describe('ReactDOMFiber', () => { | |
| ]); | ||
| }); | ||
|
|
||
| it.only('should warn for non-functional event listeners', () => { | ||
|
||
| spyOn(console, 'error'); | ||
|
Collaborator
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