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

[standards] Migrate React renderers from Haste to path-based imports #24770

Closed
ide opened this issue May 8, 2019 · 1 comment
Closed

[standards] Migrate React renderers from Haste to path-based imports #24770

ide opened this issue May 8, 2019 · 1 comment
Labels
p: Expo Partner: Expo Partner Resolution: Locked This issue was locked by the bot. Type: Discussion Long running discussion.

Comments

@ide
Copy link
Contributor

ide commented May 8, 2019

The renderer modules under Libraries/Renderer/oss/ use Haste. Since these modules are auto-generated, we need to make the renderer-generator rewrite the imports. This work mostly needs to happen in the React repo.

The code in the React repo makes assumptions about the React Native repo, like the existence of the InitializeCore module. We need to decide on an interface between the renderer in the React repo and the other modules in the React Native repo (ex: path-based imports or exposing InitializeCore through a module like react-native/Libraries/ReactPrivate that only React is allowed to import).

As part of implementing the interface, we will also need to update React's Rollup scripts and some Flow hooks to work with the new interface.

I'll take a look at the private interface approach, I think it may work out nicely and tighten up the interface in the sense that cross-repo Haste imports are an abstraction violation.

RN PR: #24782
React PR: facebook/react#15604
Sync PR: #25010

@ide ide added the Type: Discussion Long running discussion. label May 8, 2019
ide added a commit to ide/react that referenced this issue May 10, 2019
…derer

To move React Native to standard path-based imports instead of Haste, the RN renderer that is generated from the code in this repo needs to use path-based imports as well since the generated code is vendored by RN. This commit makes it so the interface between the generated renderers and RN does not rely on Haste and instead uses a private interface explicitly defined by RN. This inverts control of the abstraction so that RN decides the internals to export rather than React deciding what to import.

On RN's side, a new module named `react-native/Libraries/ReactPrivate/ReactNativePrivateInterface` explicitly exports the modules used by the renderers in this repo. (There is also a private module for InitializeCore so that we can import it just for the side effects.) On React's side, the various renderer modules access RN internals through the explicit private interface.

The Rollup configuration becomes slimmer since the only external package is now `react-native`, and the individual modules are instead listed out in `ReactNativePrivateInterface`.

Task description: facebook/react-native#24770
Sister RN PR (needs to land before this one): facebook/react-native#24782

Test Plan: Ran unit tests and Flow in this repo. Generated the renderers and manually copied them over to the RN repo. Ran the RN tests and launched the RNTester app.
ide added a commit to ide/react that referenced this issue May 10, 2019
…derer

To move React Native to standard path-based imports instead of Haste, the RN renderer that is generated from the code in this repo needs to use path-based imports as well since the generated code is vendored by RN. This commit makes it so the interface between the generated renderers and RN does not rely on Haste and instead uses a private interface explicitly defined by RN. This inverts control of the abstraction so that RN decides the internals to export rather than React deciding what to import.

On RN's side, a new module named `react-native/Libraries/ReactPrivate/ReactNativePrivateInterface` explicitly exports the modules used by the renderers in this repo. (There is also a private module for InitializeCore so that we can import it just for the side effects.) On React's side, the various renderer modules access RN internals through the explicit private interface.

The Rollup configuration becomes slimmer since the only external package is now `react-native`, and the individual modules are instead listed out in `ReactNativePrivateInterface`.

Task description: facebook/react-native#24770
Sister RN PR (needs to land before this one): facebook/react-native#24782

Test Plan: Ran unit tests and Flow in this repo. Generated the renderers and manually copied them over to the RN repo. Ran the RN tests and launched the RNTester app.
ide added a commit to ide/react that referenced this issue May 10, 2019
…derer

To move React Native to standard path-based imports instead of Haste, the RN renderer that is generated from the code in this repo needs to use path-based imports as well since the generated code is vendored by RN. This commit makes it so the interface between the generated renderers and RN does not rely on Haste and instead uses a private interface explicitly defined by RN. This inverts control of the abstraction so that RN decides the internals to export rather than React deciding what to import.

On RN's side, a new module named `react-native/Libraries/ReactPrivate/ReactNativePrivateInterface` explicitly exports the modules used by the renderers in this repo. (There is also a private module for InitializeCore so that we can import it just for the side effects.) On React's side, the various renderer modules access RN internals through the explicit private interface.

The Rollup configuration becomes slimmer since the only external package is now `react-native`, and the individual modules are instead listed out in `ReactNativePrivateInterface`.

Task description: facebook/react-native#24770
Sister RN PR (needs to land before this one): facebook/react-native#24782

Test Plan: Ran unit tests and Flow in this repo. Generated the renderers and manually copied them over to the RN repo. Ran the RN tests and launched the RNTester app.
ide added a commit to ide/react that referenced this issue May 22, 2019
…derer

To move React Native to standard path-based imports instead of Haste, the RN renderer that is generated from the code in this repo needs to use path-based imports as well since the generated code is vendored by RN. This commit makes it so the interface between the generated renderers and RN does not rely on Haste and instead uses a private interface explicitly defined by RN. This inverts control of the abstraction so that RN decides the internals to export rather than React deciding what to import.

On RN's side, a new module named `react-native/Libraries/ReactPrivate/ReactNativePrivateInterface` explicitly exports the modules used by the renderers in this repo. (There is also a private module for InitializeCore so that we can import it just for the side effects.) On React's side, the various renderer modules access RN internals through the explicit private interface.

The Rollup configuration becomes slimmer since the only external package is now `react-native`, and the individual modules are instead listed out in `ReactNativePrivateInterface`.

Task description: facebook/react-native#24770
Sister RN PR (needs to land before this one): facebook/react-native#24782

Test Plan: Ran unit tests and Flow in this repo. Generated the renderers and manually copied them over to the RN repo. Ran the RN tests and launched the RNTester app.
cpojer pushed a commit to facebook/react that referenced this issue May 23, 2019
…derer (#15604)

* [react-native] Use path-based imports instead of Haste for the RN renderer

To move React Native to standard path-based imports instead of Haste, the RN renderer that is generated from the code in this repo needs to use path-based imports as well since the generated code is vendored by RN. This commit makes it so the interface between the generated renderers and RN does not rely on Haste and instead uses a private interface explicitly defined by RN. This inverts control of the abstraction so that RN decides the internals to export rather than React deciding what to import.

On RN's side, a new module named `react-native/Libraries/ReactPrivate/ReactNativePrivateInterface` explicitly exports the modules used by the renderers in this repo. (There is also a private module for InitializeCore so that we can import it just for the side effects.) On React's side, the various renderer modules access RN internals through the explicit private interface.

The Rollup configuration becomes slimmer since the only external package is now `react-native`, and the individual modules are instead listed out in `ReactNativePrivateInterface`.

Task description: facebook/react-native#24770
Sister RN PR (needs to land before this one): facebook/react-native#24782

Test Plan: Ran unit tests and Flow in this repo. Generated the renderers and manually copied them over to the RN repo. Ran the RN tests and launched the RNTester app.

* Access natively defined "nativeFabricUIManager" instead of importing it

Some places in the Fabric renderers access `nativeFabricUIManager` (a natively defined global) instead of importing UIManager. While this is coupling across repos that depends on the timing of events, it is necessary until we have a way to defer top-level imports to run after `nativeFabricUIManager` is defined. So for consistency we use `nativeFabricUIManager` everywhere (see the comment in #15604 (review) for more context).
@ide
Copy link
Contributor Author

ide commented Jun 5, 2019

This is complete in React and RN.

@ide ide closed this as completed Jun 5, 2019
gaearon pushed a commit to gaearon/react that referenced this issue Jun 11, 2019
…derer (facebook#15604)

* [react-native] Use path-based imports instead of Haste for the RN renderer

To move React Native to standard path-based imports instead of Haste, the RN renderer that is generated from the code in this repo needs to use path-based imports as well since the generated code is vendored by RN. This commit makes it so the interface between the generated renderers and RN does not rely on Haste and instead uses a private interface explicitly defined by RN. This inverts control of the abstraction so that RN decides the internals to export rather than React deciding what to import.

On RN's side, a new module named `react-native/Libraries/ReactPrivate/ReactNativePrivateInterface` explicitly exports the modules used by the renderers in this repo. (There is also a private module for InitializeCore so that we can import it just for the side effects.) On React's side, the various renderer modules access RN internals through the explicit private interface.

The Rollup configuration becomes slimmer since the only external package is now `react-native`, and the individual modules are instead listed out in `ReactNativePrivateInterface`.

Task description: facebook/react-native#24770
Sister RN PR (needs to land before this one): facebook/react-native#24782

Test Plan: Ran unit tests and Flow in this repo. Generated the renderers and manually copied them over to the RN repo. Ran the RN tests and launched the RNTester app.

* Access natively defined "nativeFabricUIManager" instead of importing it

Some places in the Fabric renderers access `nativeFabricUIManager` (a natively defined global) instead of importing UIManager. While this is coupling across repos that depends on the timing of events, it is necessary until we have a way to defer top-level imports to run after `nativeFabricUIManager` is defined. So for consistency we use `nativeFabricUIManager` everywhere (see the comment in facebook#15604 (review) for more context).
gaearon pushed a commit to gaearon/react that referenced this issue Jun 19, 2019
…derer (facebook#15604)

* [react-native] Use path-based imports instead of Haste for the RN renderer

To move React Native to standard path-based imports instead of Haste, the RN renderer that is generated from the code in this repo needs to use path-based imports as well since the generated code is vendored by RN. This commit makes it so the interface between the generated renderers and RN does not rely on Haste and instead uses a private interface explicitly defined by RN. This inverts control of the abstraction so that RN decides the internals to export rather than React deciding what to import.

On RN's side, a new module named `react-native/Libraries/ReactPrivate/ReactNativePrivateInterface` explicitly exports the modules used by the renderers in this repo. (There is also a private module for InitializeCore so that we can import it just for the side effects.) On React's side, the various renderer modules access RN internals through the explicit private interface.

The Rollup configuration becomes slimmer since the only external package is now `react-native`, and the individual modules are instead listed out in `ReactNativePrivateInterface`.

Task description: facebook/react-native#24770
Sister RN PR (needs to land before this one): facebook/react-native#24782

Test Plan: Ran unit tests and Flow in this repo. Generated the renderers and manually copied them over to the RN repo. Ran the RN tests and launched the RNTester app.

* Access natively defined "nativeFabricUIManager" instead of importing it

Some places in the Fabric renderers access `nativeFabricUIManager` (a natively defined global) instead of importing UIManager. While this is coupling across repos that depends on the timing of events, it is necessary until we have a way to defer top-level imports to run after `nativeFabricUIManager` is defined. So for consistency we use `nativeFabricUIManager` everywhere (see the comment in facebook#15604 (review) for more context).
@facebook facebook locked as resolved and limited conversation to collaborators Jun 5, 2020
@react-native-bot react-native-bot added the Resolution: Locked This issue was locked by the bot. label Jun 5, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
p: Expo Partner: Expo Partner Resolution: Locked This issue was locked by the bot. Type: Discussion Long running discussion.
Projects
None yet
Development

No branches or pull requests

3 participants