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 when a propName looks like a mistyped event #3885

Closed
wants to merge 1 commit into from
Closed

warn when a propName looks like a mistyped event #3885

wants to merge 1 commit into from

Conversation

bloodyowl
Copy link
Contributor

closes #3548

var eventType = key.replace(/^top/, 'on');
eventTypesAccumulator[eventType.toLowerCase()] = eventType;
return eventTypesAccumulator;
}, {});
Copy link
Member

Choose a reason for hiding this comment

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

I think we probably want this in a __DEV__ block so we aren't doing extra startup work in prod (it might get dead code eliminated but not certain)

@zpao
Copy link
Member

zpao commented Jun 3, 2015

We'll need to rebase and fix a couple things but overall it seems fine to me. Any thoughts @spicyj?

@bloodyowl
Copy link
Contributor Author

fixes added @zpao

if (__DEV__) {
Object.keys(props).forEach(function(propName) {
warning(
!eventTypes.hasOwnProperty(propName),
Copy link
Contributor

Choose a reason for hiding this comment

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

This would for example not catch onMouseup. Maybe something like the following would be better?

var eventType = eventTypes[propName.toLowerCase()];
warning(
  !eventType || eventType === propName,

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good catch

how about building a regexp with the case insensitive flag when creating the map, and test anything matching the pattern, but not equal to the valid prop?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

and would fail with this edge-case:

var eventTypes = {}
var eventType = eventTypes["constructor"]
!eventType || eventType === "constructor" // false

@bloodyowl
Copy link
Contributor Author

@zpao @cody this is fixed.

@sophiebits
Copy link
Collaborator

topLevelTypes is the wrong thing to look at here. It doesn't include custom React events like onMouseEnter and onMouseLeave. Instead, you want to look at the "registration names" gathered at the top of each event "plugin":

var eventTypes = {
blur: {
phasedRegistrationNames: {
bubbled: keyOf({onBlur: true}),
captured: keyOf({onBlurCapture: true}),
},
},

These are collected in EventPluginRegistry:

https://github.com/facebook/react/blob/8f9643485d767e44af8a9c388601414a4dadf67e/src/renderers/shared/event/EventPluginRegistry.js

Hopefully that gives you some clues to go on. I also don't love that the warning logic for DOM properties

'Unknown DOM property %s. Did you mean %s?',

is in a totally different place. We should at least make the error messages a little more similar.

@sophiebits
Copy link
Collaborator

I think we're going to go with #5361.

@sophiebits sophiebits closed this Nov 3, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add warnings on incorrect casing of event handler properties
6 participants