Warn early for non-functional event listeners #10453
Conversation
To avoid triggering the non-functional event listener component
This should warn from ReactDOMFiberComponent, but we just ignore it here
aweary
changed the title
| @@ -24,6 +24,8 @@ describe('EventPluginHub', () => { | |||
| }); | |||
|
|
|||
| it('should prevent non-function listeners, at dispatch', () => { | |||
| // ReactDOMFiberComponent warns for non-function event listeners | |||
| spyOn(console, 'error'); | |||
There was a problem hiding this comment.
We should still add an expectation. So that it doesn't emit some other unwanted warning by mistake in the future.
| @@ -227,6 +236,9 @@ function setInitialDOMProperties( | |||
| // Noop | |||
| } else if (registrationNameModules.hasOwnProperty(propKey)) { | |||
| if (nextProp) { | |||
| if (__DEV__) { | |||
| warnForInvalidEventListener(propKey, nextProp); | |||
There was a problem hiding this comment.
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?
| ReactDOM.render(<Example />, container); | ||
| 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', |
There was a problem hiding this comment.
Can we provide component stack here?
|
@gaearon feedback addressed 👍 |
| '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.
This warning isn't included in stack since it fails early
| @@ -913,7 +917,7 @@ describe('ReactDOMFiber', () => { | |||
| ]); | |||
| }); | |||
|
|
|||
| it('should warn for non-functional event listeners', () => { | |||
| it.only('should warn for non-functional event listeners', () => { | |||
| spyOn(console, 'error'); | |||
| @@ -913,6 +917,24 @@ describe('ReactDOMFiber', () => { | |||
| ]); | |||
| }); | |||
|
|
|||
| it.only('should warn for non-functional event listeners', () => { | |||
There was a problem hiding this comment.
Need to fix this, otherwise should be good to go
| @@ -913,6 +917,24 @@ describe('ReactDOMFiber', () => { | |||
| ]); | |||
| }); | |||
|
|
|||
| it.only('should warn for non-functional event listeners', () => { | |||
| @@ -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.
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() { }}.
There was a problem hiding this comment.
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.
But I don't feel strongly. Didn't think about the object case where it could change.
|
This only covers ReactDOM. We have the same issue in ReactNative but there we don't need to do any early work to set up listeners so we don't have the equivalent paths. |
| 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.
Nit: I think we usually end warnings with periods (before the stack) so that they look like sentences.
Resolves #10407