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

Allow out of tree platforms to work without custom metro configs #1115

Merged
merged 6 commits into from
May 8, 2020

Conversation

acoates-ms
Copy link
Contributor

Summary:

Currently it is hard to write out of tree platforms that do the right thing, and have a single metro config. Basically as fallout from the removal of haste, now out of tree platforms contain a full implementation of react-native. In order to build an out tree platform you have to redirect any react-native import into an import into to a different package.

react-native-macos and react-native-windows do this today by specifying custom extraNodeModules settings on the metro resolver to redirect react-native to their respective packages. But that requires a different metro config to build macos vs building windows.

We have come up with a new solution that uses a custom resolveRequest function to rewrite the resolutions at bundle time, at which point we know which platform is being built, and can redirect to different packages for different platforms.

This change takes that code and generalizes it into the CLI. So now any out of tree platform can just specify the npm package that metro should use as a redirect for react-native for that platform. -- And otherwise the default metro config should work.

One note, in addition to changing the resolveRequest property, I've changed the assetRegistryPath proeprty. Previously it would be a full path, which would mean that it would not be correctly redirected by the new reactNativePlatformResolver. This path is added as a require statement in the transformer, so it should be fine for most uses as react-native/Libraries/Image/AssetRegistry

Test Plan:

Posting this early for feedback. We recently applied this change to react-native-windows and are still in the process of validating that locally. Then we'll verify that pushing this to the CLI will make everything cleaner. -- So this has not been validated yet.

Copy link
Member

@thymikee thymikee left a comment

Choose a reason for hiding this comment

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

Thanks, for this PR. I like the idea of making it easier for platform maintainers to integrate the CLI. Left a few comments on the implementation

@cpojer
Copy link
Member

cpojer commented Apr 16, 2020

This is pretty smart, I like it. Nice work @acoates-ms!

@acoates-ms
Copy link
Contributor Author

@thymikee - Getting close to this being ready I think, if you'd like to take another look.
We found an issue related to InitializeCore not getting properly run before the main module. -- But that's fixed in the latest iteration.

@thymikee
Copy link
Member

thymikee commented May 8, 2020

FYI: #1140

@grabbou
Copy link
Member

grabbou commented May 8, 2020

Thanks for the PR. Looks interesting! I like it

Currently it is hard to write out of tree platforms that do the right thing, and have a single metro config. Basically, as fallout from the removal of haste, now out of tree platforms contain a full implementation of react-native. In order to build an out tree platform you have to redirect any react-native import into an import into to a different package.

Does this mean this PR is likely to be reverted when out-of-tree platforms no longer contain a full implementation of React Native? I would guess this is our long-term goal.

@thymikee thymikee self-requested a review May 8, 2020 07:35
@Esemesek Esemesek merged commit b92fbc6 into react-native-community:master May 8, 2020
@acoates-ms
Copy link
Contributor Author

@grabbou - The long term goal would be that core is truly platform agnostic, and even android/iOS are in separate packages. But there are no near term plans to get there. -- and its not clear what the metro config would end up looking like in that case. One of the nice things about this change is that the platform has to opt in within its react-native config. So even if things change it should be easy to migrate platforms to opt back out of this behavior.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants