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

Add definitions for React Synthetic Events #161

Closed
wants to merge 4 commits into from

Conversation

jessebeach
Copy link
Collaborator

@jessebeach jessebeach commented Jan 12, 2017

React defines a set of Synthetic Events that proxy DOM events. They are handled through on* props on components. The added JSON file lists them.

https://facebook.github.io/react/docs/events.html

Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

It'd be really nice if instead of copy/pasting this file into this repo, if this was something that React just exported, so we could import it.

Many other projects, like enzyme, could use these too.

@beefancohen
Copy link
Contributor

is this consistent across all jsx-using libraries? i hope inferno and preact don't diverge here from react :/

@jessebeach
Copy link
Collaborator Author

is this consistent across all jsx-using libraries? i hope inferno and preact don't diverge here from react :/

Oh, good point. I'll check. Sometimes I forget that JSX != React :)

@jessebeach
Copy link
Collaborator Author

PReact events: https://github.com/developit/preact/blob/f0625e578c5e9a3f845a8e9461c3425647d3e4fe/src/preact.d.ts#L165

Inferno events:
https://github.com/infernojs/inferno#event-system

Inferno uses onDblClick, whereas React and PReact use onDoubleClick. I've updated the file to reflect this.

PReact seems to be missing onLoad and onError. Bizarre.

@jessebeach jessebeach force-pushed the synthetic-events branch 2 times, most recently from 63c4d9b to 3aec3f9 Compare January 13, 2017 19:34
@jessebeach
Copy link
Collaborator Author

hmm, I appear to be out of sync with the master timeline.

Fixing...

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 3aec3f9 on synthetic-events into 18587e0 on master.

@beefancohen
Copy link
Contributor

@jessebeach weird... @developit @trueadm any reason why these are different?

@jessebeach
Copy link
Collaborator Author

@evcohen @developit @trueadm, onDoubleClick is considered easier to read: facebook/react#6809

@trueadm
Copy link

trueadm commented Jan 13, 2017

@jessebeach I get that reasoning, but the aim for Preact and Inferno was to use the native naming schema that already existed. This actually has nothing to do with synthetic events either. On a side note, inferno-compat should support onDoubleClick. @LukeSheard

@jessebeach
Copy link
Collaborator Author

jessebeach commented Jan 13, 2017

This actually has nothing to do with synthetic events either

@trueadm might a better name be eventHandlers or jsxEventHandlers?

@trueadm
Copy link

trueadm commented Jan 13, 2017

@jessebeach that completely makes sense, or rather reactEventHandlers as JSX doesn't really care for naming schemas.

@ljharb
Copy link
Member

ljharb commented Jan 13, 2017

@jessebeach so is there no way that the React team could export their list from a package so we don't have to copy paste it?

@developit
Copy link

Just seconding what @trueadm noted, by default I don't think either library invent new names for existing browser events. onLoad and onError are just missing from the TypeScript definition. onDblClick should be preferred over onDoubleClick, though the latter is supported via compat.

@jessebeach
Copy link
Collaborator Author

so is there no way that the React team could export their list from a package so we don't have to copy paste it?

Looking through the React source code, I can't find a file or set of files where these DOM event handlers are defined. My best guess is they are registered through the event registry system in a way that suits their type. @trueadm is my quick-and-dirty assessment close to the truth?

I totally support you in wanting to pull this info from React rather than hard-coding it in this project. If we can get React to declare that info, I'll swap in the reference and pull out this file. But for now, I'd like to have it to build some rules off of.

@ljharb
Copy link
Member

ljharb commented Jan 14, 2017

Definitely, not blocking whatsoever if this is the best approach for now (we've had to copy-paste a number of things in Airbnb's code because React doesn't yet export them), just poking at it :-)

@jessebeach
Copy link
Collaborator Author

I named it eventHandlers.json instead of reactEventHandlers.json to keep the file unaffiliated with React specifically.

@coveralls
Copy link

coveralls commented Jan 14, 2017

Coverage Status

Coverage remained the same at 100.0% when pulling d6c04df on synthetic-events into 18587e0 on master.

@beefancohen
Copy link
Contributor

@ljharb @jessebeach i think that even if react exported this, i'd prefer to maintain our own list for 2 reasons. 1) react is not only lib that uses jsx and 2) it would make react a dependency of this project. seems odd for someone using inferno/preact/etc to have React pulled into their project by a lint plugin devDependency.

@ljharb
Copy link
Member

ljharb commented Jan 14, 2017

My ideal scenario would be that react could extract it to a separate project that both us and react (and enzyme) could import.

@lencioni
Copy link
Contributor

This JSON file seems cool, but how is/will it be used in this project? Maybe there is some magic somewhere slurping it up that we don't see in this PR?

Does this type of thing make more sense for jsx-ast-utils?

@jessebeach
Copy link
Collaborator Author

Does this type of thing make more sense for jsx-ast-utils?

Yes, in fact it does. I'll propose it there instead.

@jessebeach
Copy link
Collaborator Author

@lencioni @evcohen Moved to jsx-eslint/jsx-ast-utils#20

@jessebeach jessebeach closed this Feb 1, 2017
@ljharb ljharb deleted the synthetic-events branch February 1, 2017 21:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants