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

[WIP] Replace top-level event types with numbers #11894

Closed
wants to merge 6 commits into from

Conversation

gaearon
Copy link
Collaborator

@gaearon gaearon commented Dec 20, 2017

This is a rough proof of concept.
Not sure if it's worth it, but it removes about 0.5K post min+gzip.

The idea is to use number constants since top* strings don't have special meanings anyway. I'm using ESM exports because GCC can inline those numbers.

A few observations:

  • I like that using numbers make it clearer when we're passing "internal" event name.
  • I tried unrolling the metaprogramming added in Generate SimpleEventPlugin data structures at runtime #7616 but it makes the code larger again. However, I noticed we can use on* as the "base" name and thus ensure it stays interned (and only the rare on*Capture would be uninterned). Not sure where we compare to it so don't know if it makes a difference.
  • mediaEvents list in ReactDOMFiberComponent can be an array. There's no need to duplicate raw event names there since they already exist in BrowserEventConstants.
  • In most cases instead of the explicit raw names in BrowserEventConstants we could use "Reacty" names from SimpleEventPlugin with leading on sliced out. Not sure if there's any perf difference in passing an uninterned string to addEventListener. There's also a caveat that we don't want to listen to some SimpleEventPlugin events at top level. But we could still share a list between them if this is beneficial.

Still broken:

  • Test utils
  • Flow annotations
  • Some tests

@@ -465,42 +496,40 @@ export function setInitialProperties(
switch (tag) {
case 'iframe':
case 'object':
trapBubbledEvent('topLoad', 'load', domElement);
trapBubbledEvent(TOP_LOAD, 'load', domElement);
Copy link
Contributor

@aweary aweary Jan 8, 2018

Choose a reason for hiding this comment

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

We can remove all of these event name strings like 'load' if we have trapBubbledEvent/trapCaptureEvent call getRawEventName directly. Assuming we never call it with a mismatched top-level type and event. I don't think we do.

@philipp-spiess
Copy link
Contributor

After rebasing this work on top of the latest master, I came up with two questions:

  1. Does it really make sense to prefix all event constants with TOP_? From the comment in https://github.com/facebook/react/blob/master/packages/react-dom/src/events/BrowserEventConstants.js#L10-L16 it's clear that only some of the events are actually listened to at the top level. However the list contains more than that subset (incl. input, invalid, reset, submit, and the media events). With that in mind, I think renaming the module to EventTypes.js or InternalEventTypes.js would be more fitting. What do you think? I know that currently, the top prefix is used everywhere - Is there a reason for that?
  2. I had to make some worrisome changes to modules that contain Native in their name. Specifically when working on the Flow types here and here. Would it make sense to type the internal event type as number | string so we don't need to touch anything on the react-native side? And what part of the changed modules will also be used on the react-native side? Everything inside packages/events?

@gaearon
Copy link
Collaborator Author

gaearon commented May 23, 2018

Superseded by #12629

@gaearon gaearon closed this May 23, 2018
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.

4 participants