Use browser event names for top-level event types in React DOM #12629
Conversation
|
ReactDOM: size: -1.2%, gzip: -0.8% Details of bundled changes.Comparing: 1047980...0c6982d react-dom
Generated by 🚫 dangerJS |
| topWheel: 'wheel', | ||
| }; | ||
| export const topLevelTypes: Map<TopLevelTypes, string> = new Map([ | ||
| [ |
There was a problem hiding this comment.
Note: Map constructor doesn’t work in IE11 so we’ll need to change this
There was a problem hiding this comment.
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? :)
There was a problem hiding this comment.
Iterating sounds fine I guess
|
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? |
I can only find one file that uses this part of the private API which is 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 |
philipp-spiess
force-pushed
the
no-top-level-events
branch
from
f0c378c
to
4304b8d
Compare
|
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 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 |
|
Thank you for your input @necolas 👍 I'm wondering why the We'd not have to upstream the changes here. Instead of |
|
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. |
|
@gaearon Thanks for discussing this internally. It seems like the There are also some event names in This mapping/hybrid approach would be a more realistic approach since that PR would otherwise be a breaking change (see for example |
|
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. |
|
Events like
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 For example, the export function isStartish(topLevelType) {
return (
topLevelType === TOP_MOUSE_DOWN ||
topLevelType === TOP_TOUCH_START ||
topLevelType === "topMouseDown" ||
topLevelType === "topTouchStart"
);
}And the (few) As an alternative, changing the strings to numbers in |
|
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. |
|
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 |
|
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. |
|
Internal = internal to React |
|
Yeah 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 |
|
Sorry, I'm not sure I understand what the problem is. Can you describe it in more detail? |
|
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 With this change, we don't have to change Don't break react-native-web RNW depends on the 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 So instead of exporting |
|
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). |
|
We can review how it looks and then decide. Want to try as a part of this PR? Maybe it could be in |
|
Yup I'll definitely try that! Thank you for your input 😊 |
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
…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
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:
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 toEventTypes.jsorInternalEventTypes.jswould be more fitting. What do you think? I know that currently, thetopprefix is used everywhere - Is there a reason for that?Nativein their name. Specifically when working on the Flow types here and here. Would it make sense to type the internal event type asnumber | stringso 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 insidepackages/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 gzipreact-dom-test-utils.production.min.js (UMD_PROD)-13.2%pre,-10.2%post gzipreact-dom-unstable-native-dependencies.production.min.js (UMD_PROD)-1.2%pre,-0.6%post gzipAll 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 oldtop*strings to the new numbers as an export inReactDOMUnstableNativeDependencies. A similar mapping is already needed forReactTestUtils.js, so we have to maintain it anyhow.