Skip to content

Conversation

@rickhanlonii
Copy link
Member

Overview

This PR fixes an issue with switching between paper and fabric in React Native.

Explanation

On first initialization of React Native, ReactNativeBridgeEventPlugin.eventTypes is empty because view configs are lazily loaded. This means that the EventPluginRegistry does not run checks on events, and does not error when registering here.

If there is a second initialization (e.g. loading the fabric bundle after the paper bundle), then the eventTypes may be populated with host component events from view configs loaded at runtime.

This means that the EventPluginRegistry can error when registering the plugin if there are colliding registration names, even though these events were already in use.

This change allows the EventPluginRegistry to perform the same on initial load between all of the bundles by forcing the eventTypes to an empty object.

Justification

From the EventPluginHub doc comments, eventTypes are optional:

`eventTypes` {object}
      Optional, plugins that fire events must publish a mapping of registration
      names that are used to register listeners. Values of this mapping must
      be objects that contain `registrationName` or `phasedRegistrationNames`.

For ReactNative, we register our own top level listeners in our native host components so there is no need to register them in the EventPluginHub.

Testing

See https://fburl.com/diff/5l6zql59 for test plan.


const ReactNativeBridgeEventPlugin = {
eventTypes: eventTypes,
eventTypes: {},
Copy link
Member Author

Choose a reason for hiding this comment

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

Note: even though the docstring says this is optional, the flowtype disagress

@rickhanlonii
Copy link
Member Author

rickhanlonii commented Jun 2, 2019

Note that the EventPluginRegistry seems to be checking registration names for use in registrationNameModules, which appears to only be used in react-dom

@sizebot
Copy link

sizebot commented Jun 2, 2019

No significant bundle size changes to report.

Generated by 🚫 dangerJS

@rickhanlonii rickhanlonii changed the title Remove eventTypes from ReactNativeBridgeEventPlugin [React Native] Remove eventTypes from ReactNativeBridgeEventPlugin Jun 2, 2019
Copy link
Collaborator

@gaearon gaearon left a comment

Choose a reason for hiding this comment

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

Not super familiar with this but if they're already lazy loaded then this looks fine to me

Copy link
Contributor

@cpojer cpojer left a comment

Choose a reason for hiding this comment

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

👍

@cpojer cpojer merged commit 843a59a into facebook:master Jun 5, 2019
@rickhanlonii rickhanlonii deleted the rh-remove-event-types branch June 5, 2019 12:22
@Bonneville1979

This comment has been minimized.

facebook-github-bot pushed a commit to facebook/react-native that referenced this pull request Jun 5, 2019
Summary: This diff syncs a commit from React to bring in facebook/react#15802 (review)

Reviewed By: cpojer

Differential Revision: D15660020

fbshipit-source-id: 15d2413a69968b2898bb37d256f35bc09ebc8d58
facebook-github-bot pushed a commit to facebook/react-native that referenced this pull request Jun 19, 2019
Summary:
This isn't a full sync. It only includes cherry-picked commits for Fresh, as well as previously cherry-picked commits during last two syncs.

Changes that are already synced:

* facebook/react#15604
* facebook/react#15786
* facebook/react#15802

New changes:

* gaearon/react@5be064b
* gaearon/react@6b6d8fa
* gaearon/react@92bcf2a
* gaearon/react@7b4c2f7
* gaearon/react@39a3f2f
* gaearon/react@cabdebb
* gaearon/react@a277671
* gaearon/react@86aad18
* gaearon/react@25b3e07
* gaearon/react@ffee295
* gaearon/react@a817bc5
* gaearon/react@abb2f0f
* gaearon/react@2e20d9e
* gaearon/react@5d35024
* gaearon/react@6628573
* gaearon/react@5815bff

**This might look like a lot but note that very few of these actually touch the renderer code.** Most affect integration tests and `react-refresh` package which **has already been synced separately**.

So this is about getting in a few renderer changes that were included in those commits. That's very self-contained.

The renderer source diff is this: https://gist.github.com/gaearon/ddbda5845b4dba0d9915e2ed7f7b11e2. You can also see that in prod bundles below there's only one meaningful change (`type` used instead of `elementType`) which in that case is equivalent.

Reviewed By: rickhanlonii

Differential Revision: D15898205

fbshipit-source-id: 19619f4ff01f24765613f19e826b0199485d81bb
vovkasm pushed a commit to vovkasm/react-native that referenced this pull request Aug 7, 2019
Summary: This diff syncs a commit from React to bring in facebook/react#15802 (review)

Reviewed By: cpojer

Differential Revision: D15660020

fbshipit-source-id: 15d2413a69968b2898bb37d256f35bc09ebc8d58
(cherry picked from commit c1e03b3)
M-i-k-e-l pushed a commit to M-i-k-e-l/react-native that referenced this pull request Mar 10, 2020
Summary: This diff syncs a commit from React to bring in facebook/react#15802 (review)

Reviewed By: cpojer

Differential Revision: D15660020

fbshipit-source-id: 15d2413a69968b2898bb37d256f35bc09ebc8d58
M-i-k-e-l pushed a commit to M-i-k-e-l/react-native that referenced this pull request Mar 10, 2020
Summary:
This isn't a full sync. It only includes cherry-picked commits for Fresh, as well as previously cherry-picked commits during last two syncs.

Changes that are already synced:

* facebook/react#15604
* facebook/react#15786
* facebook/react#15802

New changes:

* gaearon/react@5be064b
* gaearon/react@6b6d8fa
* gaearon/react@92bcf2a
* gaearon/react@7b4c2f7
* gaearon/react@39a3f2f
* gaearon/react@cabdebb
* gaearon/react@a277671
* gaearon/react@86aad18
* gaearon/react@25b3e07
* gaearon/react@ffee295
* gaearon/react@a817bc5
* gaearon/react@abb2f0f
* gaearon/react@2e20d9e
* gaearon/react@5d35024
* gaearon/react@6628573
* gaearon/react@5815bff

**This might look like a lot but note that very few of these actually touch the renderer code.** Most affect integration tests and `react-refresh` package which **has already been synced separately**.

So this is about getting in a few renderer changes that were included in those commits. That's very self-contained.

The renderer source diff is this: https://gist.github.com/gaearon/ddbda5845b4dba0d9915e2ed7f7b11e2. You can also see that in prod bundles below there's only one meaningful change (`type` used instead of `elementType`) which in that case is equivalent.

Reviewed By: rickhanlonii

Differential Revision: D15898205

fbshipit-source-id: 19619f4ff01f24765613f19e826b0199485d81bb
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.

6 participants