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

Use browser event names for top-level event types in React DOM #12629

Merged
merged 46 commits into from May 15, 2018

Conversation

philipp-spiess
Copy link
Contributor

This PR builds on top of the great work of @gaearon in #11894. The main idea is to get rid of all top* strings used to identify internal events and replace them with numerical values which the compiler can inline.

Before merging this, I have two other questions which I've also asked here:

  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?

State of this PR

Besides the open questions, this PR works fine for react-dom. The difference in the bundle size is significant (well, at least enough to land #12507 🙈):

  • react-dom.production.min.js (UMD_PROD) -1.5% pre, -0.9% post gzip
  • react-dom-test-utils.production.min.js (UMD_PROD) -13.2% pre, -10.2% post gzip
  • react-dom-unstable-native-dependencies.production.min.js (UMD_PROD) -1.2% pre, -0.6% post gzip

All dom related unit tests are green and the fixtures behave as described.

There's currently one failing unit test suite (ReactNativeEvents-test.internal.js). We'll probably need to clearly look at the react-native boundaries here to see how/if we can keep this change to the DOM side.

Some libraries that depend on this private API will need adjustment as well (e.g. this file in react-native-web). Can we do anything to make that easier? We could, for example, create a mapping from the old top* strings to the new numbers as an export in ReactDOMUnstableNativeDependencies. A similar mapping is already needed for ReactTestUtils.js, so we have to maintain it anyhow.

@pull-bot
Copy link

pull-bot commented Apr 17, 2018

ReactDOM: size: -1.2%, gzip: -0.8%

Details of bundled changes.

Comparing: 1047980...0c6982d

react-dom

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-dom.development.js +0.2% +0.4% 642.7 KB 643.99 KB 147.67 KB 148.23 KB UMD_DEV
react-dom.production.min.js -1.2% -0.8% 102.3 KB 101.12 KB 32.62 KB 32.36 KB UMD_PROD
react-dom.development.js +0.2% +0.4% 627.06 KB 628.35 KB 143.53 KB 144.15 KB NODE_DEV
react-dom.production.min.js -1.2% -0.9% 100.71 KB 99.53 KB 31.73 KB 31.45 KB NODE_PROD
react-dom-test-utils.development.js +4.5% +4.5% 40.71 KB 42.52 KB 11.68 KB 12.21 KB UMD_DEV
react-dom-test-utils.production.min.js -1.6% -2.5% 10.59 KB 10.42 KB 3.93 KB 3.84 KB UMD_PROD
react-dom-test-utils.development.js +5.1% +5.1% 35.57 KB 37.38 KB 10.26 KB 10.79 KB NODE_DEV
react-dom-test-utils.production.min.js -1.7% -2.6% 9.86 KB 9.69 KB 3.72 KB 3.62 KB NODE_PROD
react-dom-unstable-native-dependencies.development.js +0.4% +0.9% 61.18 KB 61.45 KB 16.02 KB 16.17 KB UMD_DEV
react-dom-unstable-native-dependencies.production.min.js 🔺+0.5% 🔺+0.4% 11.31 KB 11.36 KB 3.91 KB 3.92 KB UMD_PROD
react-dom-unstable-native-dependencies.development.js +0.5% +0.9% 56.84 KB 57.11 KB 14.77 KB 14.9 KB NODE_DEV
react-dom-unstable-native-dependencies.production.min.js 🔺+0.5% 🔺+0.2% 10.65 KB 10.71 KB 3.67 KB 3.67 KB NODE_PROD
react-dom-server.browser.development.js +0.4% +0.2% 101.58 KB 101.97 KB 26.54 KB 26.6 KB UMD_DEV
react-dom-server.browser.development.js +0.4% +0.2% 90.88 KB 91.27 KB 24.28 KB 24.34 KB NODE_DEV
react-dom-server.node.development.js +0.4% +0.2% 92.8 KB 93.2 KB 24.83 KB 24.88 KB NODE_DEV
ReactDOM-dev.js +0.2% +0.5% 652.26 KB 653.78 KB 146.43 KB 147.13 KB FB_WWW_DEV
ReactDOM-prod.js -0.5% -0.4% 303.85 KB 302.4 KB 55.11 KB 54.91 KB FB_WWW_PROD
ReactTestUtils-dev.js +5.2% +5.1% 36.85 KB 38.75 KB 10.45 KB 10.98 KB FB_WWW_DEV
ReactDOMUnstableNativeDependencies-dev.js +0.4% +0.9% 57.09 KB 57.32 KB 14.56 KB 14.69 KB FB_WWW_DEV
ReactDOMUnstableNativeDependencies-prod.js 🔺+0.1% 🔺+0.1% 26.33 KB 26.35 KB 5.37 KB 5.37 KB FB_WWW_PROD
ReactDOMServer-dev.js +0.6% +0.3% 94.43 KB 94.98 KB 24.1 KB 24.17 KB FB_WWW_DEV

Generated by 🚫 dangerJS

topWheel: 'wheel',
};
export const topLevelTypes: Map<TopLevelTypes, string> = new Map([
[
Copy link
Collaborator

Choose a reason for hiding this comment

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

Note: Map constructor doesn’t work in IE11 so we’ll need to change this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ugh. Thanks for pointing that out. Should I just create the nested array structure and loop over it to set the props then or do you have a util function for that? :)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Iterating sounds fine I guess

@gaearon
Copy link
Collaborator

gaearon commented Apr 17, 2018

Nice work.

I’ll chat to somebody on the team about RN and follow up with you.

Can you give me an idea of how breaking those changes would be for React Native Web? What exactly would they need to change?

@philipp-spiess
Copy link
Contributor Author

Can you give me an idea of how breaking those changes would be for React Native Web? What exactly would they need to change?

I can only find one file that uses this part of the private API which is injectResponderEventPlugin/index.js. In there, all we'd have to is change the top* strings to the numbers (where an additional private export could help, e.g TopLevelTypes['touchStart'].

Maybe @necolas can skim in here and help me out there as well? I can also sit down and try to make a PR with the hard coded number values and set up an example app :)

@necolas sorry for pinging you here out of context! We're thinking about ways to shrink the react-dom bundle size and getting rid of the top* strings would help quite a bit. We know that react-native-web uses private API to enable some of the react-native specific event plugins and want to make a smooth migration path for your project. I'd gladly set up the PR but we first need to find out the scope of this change. Thanks ❤️

@necolas
Copy link
Contributor

necolas commented Apr 17, 2018

FWIW, these top-level event types used to be available but at some point on the way to flattening the React bundles I had to switch to using the strings which this PR is touching
necolas/react-native-web@44ecbc0#diff-fecd5b3cfaeb4f62d70f4219e850f039

Another option is to upstream some or all of the changes I have to make to the responder plugin event dependencies and then I don't need to know about these event internals anymore

@philipp-spiess
Copy link
Contributor Author

Thank you for your input @necolas 👍

I'm wondering why the ResponderEventPlugin does not define dependencies like the other plugins do. @gaearon Do you think we break anything on the react-native side if we add those?

We'd not have to upstream the changes here. Instead of topLevelType we could make the changes based on nativeEvent.type instead as well.

@gaearon
Copy link
Collaborator

gaearon commented Apr 17, 2018

We chatted a bit about this today with the team. We won't have time to fix RN ourselves but if you can send PRs to it to also convert to numbers (and then send a PR to fix RNW) then we can take this.

@philipp-spiess
Copy link
Contributor Author

@gaearon Thanks for discussing this internally.

It seems like the top* event strings are quite scattered in react-native (I could find 126 matches for the regex /("|')top[a-zA-Z]+/ - strangely most of the strings are only used in Java modules and I can't find the C# counter part). To avoid magic numbers everywhere, we could still use the top* strings there and set up a map to the numbers in JavaScript (or, alternatively, support both types number for react-dom and string for react-native).

There are also some event names in react-native that we don't have in TopLevelEventTypes yet (e.g. topMomentumScrollEnd). I wonder if it would be enough to generate a new number whenever the cache misses?

This mapping/hybrid approach would be a more realistic approach since that PR would otherwise be a breaking change (see for example react-native-windows which also depends on the top* strings).

@gaearon
Copy link
Collaborator

gaearon commented Apr 18, 2018

Have you verified those strings have the same role? It could be that they just picked up a convention but don't actually use it the same way in the event system.

@philipp-spiess
Copy link
Contributor Author

Events like topMomentumScrollBegin are definitely handled in the event system. The native modules call into receiveEvent with the topLevelType string. From there, it will go to extractEvents which will call the individual event plugins including the ResponderEventPlugin which is now changed to work with number instead of the top* strings.

screen shot 2018-04-19 at 01 19 20

Something that’s interesting is that the native platforms (iOS, Android) will register their events in these objects.

If we would keep the strings in react-native but use numerical IDs for react-dom, all event plugins that would be used in dom and native (which currently is limited to ResponderEventPlugin) would need to handle both types. That certainly would be the easiest part but handling two event id cases seems very odd.

For example, the isStartish helper will need to look like this:

export function isStartish(topLevelType) {
  return (
    topLevelType === TOP_MOUSE_DOWN ||
    topLevelType === TOP_TOUCH_START ||
    topLevelType === "topMouseDown" ||
    topLevelType === "topTouchStart"
  );
}

And the (few) top* strings would again be part of the dom artifact.

As an alternative, changing the strings to numbers in react-native would be a lot of work (since, as I mentioned, the strings are quire scattered) and we'd have to find a way how we can maintain only one map (TopLevelEventTypes) and use it in iOS and Android.

@gaearon
Copy link
Collaborator

gaearon commented Apr 19, 2018

Is ResponderEventPlugin only used by RNW? Maybe we could just fork it. RN would have its own version, RNW would have another one, and our repo wouldn’t have either.

@necolas
Copy link
Contributor

necolas commented Apr 19, 2018

IIRC that was discussed when React was flattening bundles, and ResponderEventPlugin had too many internal dependencies for it to be practical to handle another way

@gaearon
Copy link
Collaborator

gaearon commented Apr 19, 2018

Maybe I'm wrong but I don't see anything depending on ResponderEventPlugin on the web at FB.

Does TapEventPlugin depend on it? Even if it does, we are ready to remove those deps now.

@necolas
Copy link
Contributor

necolas commented Apr 19, 2018

Internal = internal to React

@philipp-spiess
Copy link
Contributor Author

Yeah ResponderEventPlugin requires a few internal dependencies. That would not be a problem when moving it from packages/events to packages/react-native-renderer (which is also where the other react native event plugins reside).

To make it work as a fork in RNW we'd probably have to add a few exports here.

When doing that, I think we should also move all other web specific event plugins to packages/react-dom so we don't start mixing them up.

@gaearon
Copy link
Collaborator

gaearon commented Apr 19, 2018

Sorry, I'm not sure I understand what the problem is. Can you describe it in more detail?

@philipp-spiess
Copy link
Contributor Author

philipp-spiess commented Apr 19, 2018

Sorry for not being clear enough. There are essentially two issues here which we're trying to solve: react-native and react-native-web support.

Don't break react-native

I like your idea of moving ResponderEventPlugin away from the commonly shared event code. I suggest we move it into packages/react-native-renderer where the second react native event plugin is also located (so it would still be part of the react GitHub repo but no longer accessible for react-dom

With this change, we don't have to change ResponderEventPlugin and can keep using string event identifiers there without affecting the DOM side. The shared event code (if any) will need to handle both numbers and strings as event IDs but it's up to the renderer to use whatever they want.

Don't break react-native-web

RNW depends on the ResponderEventPlugin so we'll have to make a DOM fork (directly as part of RNW github repo?) to make it work again.

Since this plugin requires some React internal API (helper functions to dispatch events, being able to access the dom top level event types map, etc), we also need to adjust the exported private API of ReactDOMUnstableNativeDependencies so that its possible to implement that event plugin.

So instead of exporting ResponderEventPlugin, ReactDOMUnstableNativeDependencies will now export everything that's required to build-your-own responder plugin.

@gaearon
Copy link
Collaborator

gaearon commented Apr 19, 2018

Exporting more doesn't sound great. Perhaps we can have two forks in our own repo?

@philipp-spiess
Copy link
Contributor Author

philipp-spiess commented Apr 19, 2018

Exporting more doesn't sound great. Perhaps we can have two forks in our own repo?

Yeah we can do that as well. The question is we want to maintain two forks of this module where the only difference is that they use different event identifiers (well, at least we can upstream the changes from RNW without worrying about breaking RN :D).

@gaearon
Copy link
Collaborator

gaearon commented Apr 19, 2018

We can review how it looks and then decide. Want to try as a part of this PR?

Maybe it could be in shared folder and have the IDs injected.

@philipp-spiess
Copy link
Contributor Author

Yup I'll definitely try that! Thank you for your input 😊

philipp-spiess added a commit to philipp-spiess/react that referenced this pull request Jun 12, 2018
In facebook#12629 @gaearon suggested that it would be better to drop usage of
`ReactTestUtils.Simulate` and `ReactTestUtils.SimulateNative`. In this
PR I’m attempting at removing it from a lot of places with only a few
leftovers.

Those leftovers can be categorized into three groups:

1. Anything that tests that `SimulateNative` throws. This is a property
   that native event dispatching doesn’t have so I can’t convert that
   easily. Affected test suites: `EventPluginHub-test`,
   `ReactBrowserEventEmitter-test`.
2. Anything that tests `ReactTestUtils` directly. Affected test suites:
   `ReactBrowserEventEmitter-test` (this file has one test that reads
    "should have mouse enter simulated by test utils"),
    `ReactTestUtils-test`.
3. Anything that dispatches a `change` event. The reason here goes a bit
   deeper and is rooted in the way we shim onChange. Usually when using
   native event dispatching, you would set the node’s `.value` and then
   dispatch the event. However inside [`inputValueTracking.js`][] we
   install a setter on the node’s `.value` that will ignore the next
   `change` event (I found [this][near-perfect-oninput-shim] article
   from Sophie that explains that this is to avoid onChange when
   updating the value via JavaScript).

All remaining usages of `Simulate` or `SimulateNative` can be avoided
by mounting the containers inside the `document` and dispatching native
events.

Here some remarks:

1. I’m using `Element#click()` instead of `dispatchEvent`. In the jsdom
   changelog I read that `click()` now properly sets the correct values
   (you can also verify it does the same thing by looking at the
   [source][jsdom-source]).
2. I had to update jsdom in order to get `TouchEvent` constructors
   working (and while doing so also updated jest). There was one
   unexpected surprise: `ReactScheduler-test` was relying on not having
   `window.performance` available. I’ve recreated the previous
   environment by deleting this property from the global object.
3. I was a bit confused that `ReactTestUtils.renderIntoDocument()` does
   not render into the document 🤷‍

[`inputValueTracking.js`]: https://github.com/facebook/react/blob/392530104c00c25074ce38e1f7e1dd363018c7ce/packages/react-dom/src/client/inputValueTracking.js#L79
[near-perfect-oninput-shim]: https://sophiebits.com/2013/06/18/a-near-perfect-oninput-shim-for-ie-8-and-9.html
[jsdom-source]: https://github.com/jsdom/jsdom/blob/45b77f5d21cef74cad278d089937d8462c29acce/lib/jsdom/living/nodes/HTMLElement-impl.js#L43-L76
gaearon pushed a commit that referenced this pull request Jun 13, 2018
…13023)

* Use native event dispatching instead of Simulate or SimulateNative

In #12629 @gaearon suggested that it would be better to drop usage of
`ReactTestUtils.Simulate` and `ReactTestUtils.SimulateNative`. In this
PR I’m attempting at removing it from a lot of places with only a few
leftovers.

Those leftovers can be categorized into three groups:

1. Anything that tests that `SimulateNative` throws. This is a property
   that native event dispatching doesn’t have so I can’t convert that
   easily. Affected test suites: `EventPluginHub-test`,
   `ReactBrowserEventEmitter-test`.
2. Anything that tests `ReactTestUtils` directly. Affected test suites:
   `ReactBrowserEventEmitter-test` (this file has one test that reads
    "should have mouse enter simulated by test utils"),
    `ReactTestUtils-test`.
3. Anything that dispatches a `change` event. The reason here goes a bit
   deeper and is rooted in the way we shim onChange. Usually when using
   native event dispatching, you would set the node’s `.value` and then
   dispatch the event. However inside [`inputValueTracking.js`][] we
   install a setter on the node’s `.value` that will ignore the next
   `change` event (I found [this][near-perfect-oninput-shim] article
   from Sophie that explains that this is to avoid onChange when
   updating the value via JavaScript).

All remaining usages of `Simulate` or `SimulateNative` can be avoided
by mounting the containers inside the `document` and dispatching native
events.

Here some remarks:

1. I’m using `Element#click()` instead of `dispatchEvent`. In the jsdom
   changelog I read that `click()` now properly sets the correct values
   (you can also verify it does the same thing by looking at the
   [source][jsdom-source]).
2. I had to update jsdom in order to get `TouchEvent` constructors
   working (and while doing so also updated jest). There was one
   unexpected surprise: `ReactScheduler-test` was relying on not having
   `window.performance` available. I’ve recreated the previous
   environment by deleting this property from the global object.
3. I was a bit confused that `ReactTestUtils.renderIntoDocument()` does
   not render into the document 🤷‍

[`inputValueTracking.js`]: https://github.com/facebook/react/blob/392530104c00c25074ce38e1f7e1dd363018c7ce/packages/react-dom/src/client/inputValueTracking.js#L79
[near-perfect-oninput-shim]: https://sophiebits.com/2013/06/18/a-near-perfect-oninput-shim-for-ie-8-and-9.html
[jsdom-source]: https://github.com/jsdom/jsdom/blob/45b77f5d21cef74cad278d089937d8462c29acce/lib/jsdom/living/nodes/HTMLElement-impl.js#L43-L76

* Make sure contains are unlinked from the document even if the test fails

* Remove unnecessary findDOMNode calls
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.

None yet

6 participants