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

This is an alternate metro bundler fix for multiple out of tree platforms #4576

Merged
merged 8 commits into from
Apr 15, 2020

Conversation

acoates-ms
Copy link
Contributor

@acoates-ms acoates-ms commented Apr 12, 2020

Fixes #4081

This solution is much less invasive than the one in #4522 and I think could be easily added to the react-native CLI to avoid any need for any custom metro config. The CLI could change its default metro config to include this resolver and automatically add all the platforms installed to the config.

Microsoft Reviewers: Open in CodeFlow

@acoates-ms acoates-ms requested a review from a team as a code owner April 12, 2020 01:29
@kmelmon
Copy link
Contributor

kmelmon commented Apr 12, 2020

I like the simplicity of this solution! Some questions:

  1. It looks like this solution requires every out of tree platform to provide a complete implementation of react-native. So for example react-native-windows would ship with a copy of react-native (like it does today), so would react-native-macos, etc. This lack of sharing has consequences:
    A) The main one I can think of is, in order to support every patch version of react-native that's been released, we'd need to also ship a matching patch version, otherwise we'd be telling a lie by installing a copy of react-native that's actually from a previous release. I suppose on the flip side of this, if we go with my solution, we'd need some guarantee that a given patch version of react-native is compatible with some version of ours, and I haven't worked out how to make that guarantee yet.
    B) Another consequence of not sharing is the additional code that needs to be installed and stored in the haste map. Probably not a huge deal but there is some overhead.
    C) It feels a bit strange for the developer to install react-native but never pull in any of the code from node_modules/react-native into their bundle. This could cause customer confusion.
  2. I've seen the resolve function called in the context of resolving a relative path. I'm trying to convince myself this will "just work"... I guess the base cases is a path comes in with 'react-native' in it, we'll point it over to the platform-specific version, and so in the relative path case, all relative paths will be relative to something we've already resolved. Does that make sense?
  3. Does the above mean we don't need any blacklist no matter how many out-of-tree platforms are installed? Hopefully yes. #Closed

@acoates-ms
Copy link
Contributor Author

acoates-ms commented Apr 13, 2020

  1. Yes we still have to ship a full RN copy
  2. Relative paths should be fine. The only time that wouldn't work is if the relative path was crossing package boundaries, which is invalid anyway.
  3. Yes no more blacklists needed. #Closed

@kmelmon
Copy link
Contributor

kmelmon commented Apr 13, 2020

Thanks. I was planning on waiting until 0.62 for the version I was working on as well so we still have some time to decide. I'll post an update on the main proposal and ask for thoughts from facebook folks to help us decide. #Closed

vnext/metro.config.js Outdated Show resolved Hide resolved
@kmelmon
Copy link
Contributor

kmelmon commented Apr 14, 2020

Question: What should we do with react-native.config.js? Particularly for the CLI template app, which points it directly at react-native-windows, this would be wrong for other platforms. #Closed

@acoates-ms
Copy link
Contributor Author

acoates-ms commented Apr 14, 2020

I think we can actually push this change into the react-native cli, which provides its own metro config defaults that get overriden by the apps.

What I'm thinking of is adding another property to the platform object provided by out of tree platforms in the react-native.config.js. This property would be something like

    platforms: {
      windows: {
        linkConfig: () => null,
        projectConfig: projectConfig.projectConfigWindows,
        dependencyConfig: dependencyConfig.dependencyConfigWindows,
        metroReactNativePackage: 'react-native-windows'
      },
    },

(the metroReactNativePackage being the new property)

Then the CLI can add the resolveRequest property to the default metro config based on what platforms are installed.

Then I think we wont need to modify the metro.config at all in our template going forward.

#Closed

@kmelmon
Copy link
Contributor

kmelmon commented Apr 14, 2020

Thanks. What happens if you build/run a sample app for another platform with this change? It seems like it will use react-native-windows for various properties, particularly assetRegistryPath, which could mess up resolution of assets. When I got this property wrong in my testing, it resulted in the app not rendering any images that were require'd in as assets (like the background React logo image in the CLI template app)


In reply to: 613629109 [](ancestors = 613629109)

@acoates-ms
Copy link
Contributor Author

acoates-ms commented Apr 14, 2020

When building for other platforms, this should be a no-op change, since the custom resolver will just call the default resolver. Do we have a good test case for an asset that we know is currently working? I can double check its still working. #Closed

@kmelmon
Copy link
Contributor

kmelmon commented Apr 14, 2020

I believe this will be a change for the CLI app when building for android. Previously, we would redirect resolve to react-native-windows, now we will resolve it to react-native. However reactNativePath will still point to react-native-windows. Thus my concern.

The template CLI app has an image in the background near the top (the React logo). That should be a good test.


In reply to: 613645515 [](ancestors = 613645515)

Copy link
Contributor

@kmelmon kmelmon left a comment

Choose a reason for hiding this comment

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

:shipit:

@acoates-ms
Copy link
Contributor Author

Just as a note, here is what I think the CLI change could look like:
react-native-community/cli#1115

@acoates-ms acoates-ms merged commit d53d06a into microsoft:master Apr 15, 2020
@acoates-ms acoates-ms deleted the altmetrofix branch April 15, 2020 22:22
asklar pushed a commit to asklar/react-native-windows that referenced this pull request Apr 19, 2020
…orms (microsoft#4576)

* Use a custom resolver wrapper to allow metro to run for multiple platforms at once

* Change files

* More simplification of metro configs

* CI fixes

* Fix bundle command on init app to properly use windows platform

* More simplification of metro configs

* Fix asset loading
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.

Adding windows to a project breaks Metro configuration for other platforms
2 participants