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

Improve Metro support for out-of-tree platforms #182

Open
kmelmon opened this issue Jan 15, 2020 · 31 comments
Open

Improve Metro support for out-of-tree platforms #182

kmelmon opened this issue Jan 15, 2020 · 31 comments
Labels
🗣 Discussion This label identifies an ongoing discussion on a subject

Comments

@kmelmon
Copy link

kmelmon commented Jan 15, 2020

Currently, react-native-windows needs to create an overridden metro.config.js due to the windows platform having a bunch of platform overrides. This makes it difficult for customers to target multiple platforms.... Once react-native-windows is added to an existing project, their metro.config.js gets changed to something that works for windows, but doesn't work for other platforms.

I don't have a proposal yet so I'm opening this feature request to kick-start the discussion of how to make it more seamless for metro to work with out-of-tree platforms. What changes could we make in metro to enable this? For example, could we add a platform parameter that metro understands which would allow a single metro.config.js to be written that targets multiple platforms?

@kevinvangelder
Copy link

This was discussed briefly in the Slack (and possibly the Discord) before this repo was created, and I think there were some suggested solutions. Maybe someone could look through the archives?

@tom-un
Copy link

tom-un commented Jan 16, 2020

The other out-of-tree platform, react-native-macos, has the same need. The react-native-community repos which contain ios and android implementations may need macos and windows implementations. Repos will need to be able to consume "react-native", "react-native-macos", and "react-native-windows" and any other platform simultaneously.

I have a work-around to allow react-native and react-native-macos to coexist in the same repo, and I think it would work for react-native-windows and other out-of-tree platforms.

The work-around involves having a metro.config.js for each platform (e.g. metro.config.macos.js) and a react-native.config.js. The later looks for a --use-react-native-macos switch and when present it does two things: it pushes --config=metro.config.macos.js to use the platform specific metro config, and specifies reactNativePath: 'node_modules/react-native-macos'. The metro.config.js has to blacklist 'node_modules/react-native-macos', and conversely metro.config.macos.js has to blacklist 'node_modules/react-native'.

With this workaround, commands like cli.js start work against react-native and cli.js start --use-react-native-macos work against react-native-macos.

react-native.config.js

if (process.argv.includes('--use-react-native-macos')) {
  process.argv = process.argv.filter(arg => arg !== '--use-react-native-macos');
  process.argv.push('--config=metro.config.macos.js');
  module.exports = {
    reactNativePath: 'node_modules/react-native-macos',
  };
} 

metro.config.js

const blacklist = require('metro-config/src/defaults/blacklist');

module.exports = {
  resolver: {
    blacklistRE: blacklist([/node_modules\/react-native-macos\/.*/])
  },
};

metro.config.macos.js

const blacklist = require('metro-config/src/defaults/blacklist');
const path = require('path')

module.exports = {
  resolver: {
    platforms: ['macos', 'ios', 'android'],
    blacklistRE: blacklist([/node_modules\/react-native\/.*/])
  },
};

@kelset
Copy link
Member

kelset commented Jan 16, 2020

(just for reference, this previous conversation was sort of related to this -> #50)

@cpojer
Copy link
Member

cpojer commented Jan 17, 2020

Could somebody document the changes that are necessary for the windows and macos platforms? Based on that we can work on a plan to:

  • Reduce the differences so that the same, or mostly the same, configuration can be used.
  • Figure out a path forward for all the cases where we cannot align on a single configuration.

@kelset kelset added the 🗣 Discussion This label identifies an ongoing discussion on a subject label Jan 17, 2020
@acoates-ms
Copy link
Collaborator

Right now we are still on 0.60 so there are several hastemap differences between the platforms. but after 0.61 the main setting difference is going to be that we rewrite the react native path to be different for the out of tree platforms. @kmelmon has I think looked some at what differences we might still need in 0.61

@kmelmon
Copy link
Author

kmelmon commented Jan 22, 2020

I'm working with @ddalp on migrating react-native-windows to 61. So far the only difference we know is required is to blacklist react-native. IIUC, out of tree platform overrides (eg Dimensions.windows.js) need to be co-located with the JS they are overriding. We're currently doing this by copying all of react-native JS into react-native-windows, and then blacklisting react-native.

I'm open to discussing other ways of doing this. If there was for instance a way to tell metro explicitly where to find a given platform's overrides, we could place these overrides somewhere else. That shouldn't require metro to know anything about other platforms besides a variable saying where to look.

I believe the same will be required on MacOS. @tom-un feel free to chime in here.

@kmelmon
Copy link
Author

kmelmon commented Jan 24, 2020

@cpojer I'm trying to figure out what changed in metro between react-native 60 and 61. Can I find this in release notes somewhere, or is there a list of commits I can look at? For example, Haste module resolution is now gone, which is a major change and likely affects this proposal - I'm particularly interested in changes like these.

Any pointers would be greatly appreciated!

@kmelmon
Copy link
Author

kmelmon commented Jan 25, 2020

Update on above: I spent time debugging metro today and eventually found this change:
facebook/metro@7b46150

The change removed support for the 'providesModuleNodeModules' prop from metro.config.js. Removing this had the subtle side-effect of turning off Haste Module resolution (in DependencyGraphHelper.js, isNodeModulesDir()), and thus all Haste style require's stopped working.

react-native-windows was using 'providesModuleNodeModules' so this explains why the change broke things for us when we upgraded to 61.

@kmelmon
Copy link
Author

kmelmon commented Feb 4, 2020

@cpojer this is what we're left with as a typical metro.config.js for windows. I believe the same is true of MacOS.

resolver: {
extraNodeModules: {
// Redirect react-native to react-native-windows
'react-native': rnwPath,
'react-native-windows': rnwPath,
},
// Since there are multiple copies of react-native, we need to ensure that metro only sees one of them
blacklistRE: blacklist([
new RegExp(
${(path.resolve(rnPath) + path.sep).replace(/[/\\\\]/g, '[/\\\\]')}.*,
),

The jist of this is:
We copy all the JS from react-native into react-native-windows, which contains platform overrides for whatever we needed to fork (eg Dimensions.windows.js).
Then in metro.config,js:
A) Redirect react-native to react-native-windows
B) Blacklist react-native so that it's not found in two places

@cpojer
Copy link
Member

cpojer commented Feb 5, 2020

Thanks for the summary. I kinda feel like we should figure out a way to make this work first-class or automatic inside of Metro so that this configuration is not needed.

@grabbou
Copy link
Member

grabbou commented Feb 13, 2020

Well, CLI might be the place to do it then. My long-standing principle is that things should "just work" and that's why I've been particularly excited about "autolinking".

Right now, we detect "platforms" and they can enhance the CLI with additional commands, such as run-windows. Aside from that, we automatically tell Metro to respect platform while resolving files.

I am pretty sure we could automatically do extraNodeModules and blacklistRE, by either letting each platform define a custom function, e.g. customiseMetroConfig inside their react-native.config.js (might be tricky with multiple platforms) or building a small abstraction inside CLI to do it automatically.

That way, they can ship this as a part of the package w/o asking users to perform the changes.

@grabbou
Copy link
Member

grabbou commented Feb 13, 2020

Once we know what's the standard way to integrate a platform as per @cpojer comment, we can take it as a base and implement support inside of the CLI.

@kmelmon
Copy link
Author

kmelmon commented Mar 10, 2020

@grabbou I'd like to find a solution that would allow a single instance of metro to serve up a bundle for any platform at any time. This 'gold standard' would allow a developer to have, say, both an android device as well as a windows device running their app and they could see live updates to both apps as they iterate on their code.

My current understanding is that the blacklist is configured when metro is first started up, so I think this means it can only be configured to serve up a bundle for one platform at a time. Is that correct? If so, is there any change that can be done in the CLI itself or does this change need to be done in metro instead?

@elicwhite
Copy link

I believe that an existing React Native app the same Metro instance can serve both the iOS and Android bundles. I’m not sure what the config is that supports that, but I’m pretty positive that works.

@kmelmon
Copy link
Author

kmelmon commented Mar 10, 2020

Yes with a normal configuration metro can serve up bundles for both ios and android. The question is what happens once the metro config has been overridden for an out-of-tree platform. The current approach for out-of-tree platforms is to set these two properties in the metro config:
-extraNodeModules
-blacklistRE
It seems that these properties can only be set as metro starts up, and from that point forward metro will always use that configuration. I'm guessing here but it seems like if that's true, then the idea that we can use different configs doesn't quite work - we would need to build more flexibility into metro. I'm basically asking how much flexibility is there in these two properties? Can they be re-configured as a bundle is being requested? We would likely need that.

@cpojer
Copy link
Member

cpojer commented Mar 10, 2020

Unfortunately these options are per Metro instance and cannot be reconfigured dynamically.

@kmelmon
Copy link
Author

kmelmon commented Mar 11, 2020

Thanks! Given that, I'm leaning towards changing metro to address this. I'm now considering two possible approaches:

  1. Implement redirection/blacklist per platform. This would likely introduce new properties that would control what happens during the resolver stage. With this option, out-of-tree platforms would still make a copy of react-native and blacklist react-native when building a bundle for that platform.
  2. Implement a new mechanism. The basic idea here would be for out-of-tree platforms to put their overrides somewhere other than react-native, and configure metro to look for these overrides in this location.

I am leaning towards option 2 at this point as option 1 still requires out-of-tree platforms to make a copy of react-native which comes with consequences.

@acoates-ms
Copy link
Collaborator

Option 2 would basically involve introducing a custom module resolution logic. We just got rid of that (haste modules), I wouldn't want to go introduce another custom module resolution (not to mention that such a thing would have carry on affect to the rest of the tooling (jest, typescript, etc)

I suspect we need to do something similar to what haul does, where the bundle server essentially runs multiple instances of the bunder, and directs calls to them based on the requested URL (and which platform it specifies) Whether that is done internally to metro, or as a wrapper around metro would be the real question.

@kmelmon
Copy link
Author

kmelmon commented Mar 11, 2020

Well, the bundling process already does something special to resolve modules, based on the platform (eg when building for platform = windows, instead of loading View.js, it will load View.windows.js if one exists). This file currently has to exist side-by-side with the file it's overriding, and this restriction has led to what we're currently doing. Why not just loosen the restriction? For example why not allow the override to be found in the same relative path, but with a different root?

For example, when resolving:
node_modules\react-native\Libraries\Components\View\View.js
if we're buildilng a bundle for platform = windows, look for an override here:
node_modules\react-native-windows\Libraries\Components\View\View.js
or perhaps be even more explicit and look for:
node_modules\react-native-windows\Libraries\Components\View\View.windows.js

This would avoid copying/redirecting all of react-native as well as the blacklist.

@cpojer
Copy link
Member

cpojer commented Mar 12, 2020

Neither of these options seem workable to me. I think what we really need to do is change React Native, not Metro, and get rid of the custom configuration. If Metro can handle Android and iOS in a single instance, there is no reason it couldn't handle any platform. The custom config is an artifact of how we implement out-of-three platforms. Let's figure out how to turn all platforms into out of tree platforms and support them first-class.

@kmelmon
Copy link
Author

kmelmon commented Mar 23, 2020

Update: I talked over goals at a high level with folks here at Microsoft, and spent time debugging through the metro resolver to get a better idea of how it resolves platform overrides. I learned a bunch and have a new proposal, outlined below.

Regarding the out-of-tree platforms Microsoft currently has:
-react-native-windows has JS overrides in many places. Essentially, this out-of-tree platform needs the ability to override any JS file that ships with react-native.
-react-native-win32 works very similarly to react-native-windows. This flavor of react-native for windows may only ever be used internally by Office, but we want it to work the same way.
-react-native-macos was done as a fork. Instead of providing .macos overrides, the platform agnostic JS files were directly changed, and installing react-native-macos means you're installing a fork of react-native that only works on macos. This made it easier to upgrade but leaves us with the same problem of having multiple versions of react-native installed on a user's system. We want to change this to work like react-native-windows, and this seems doable - we now have tooling in place for Windows that can apply diffs of platform overrides whenever we upgrade. This would make macos no different than the other platforms. I'm discussing this with folks internally.

So the most basic requirement for out-of-tree platforms is that they need to provide the bundler with platform overrides for potentially any JS file that's part of react-native.

I studied the metro resolver code to get an idea of how it works and what our options are. I now see that adding custom resolver logic would be problematic and difficult, particularly for relative path imports, which the resolver assumes can be found in a location relative to the file it's being imported from. The solution that seems most straightforward is to ensure all platform overrides live side-by-side with the JS files they are overriding.

Currently, react-native-windows solves this by copying react-native into react-native-windows, and blacklisting react-native. We don't like this solution as it doesn't share react-native with other out-of-tree platforms, and leads to needing a custom metro config. So we're now considering a different solution, which is to copy the platform overrides into react-native (likely when the out-of-tree platform is installed). This eliminates the need for a custom metro config. It also means ios/android don't need to change at all as they already provide platform overrides in the same place.

My next step will be to try out this solution on react-native-windows to ferret out any other issues. We'll need to do several things to make this robust, such as making sure only overrides are copied, upgrading is handled properly, etc.

@elicwhite
Copy link

By copying react Native Windows into React Native do you mean literally copying it into the react native node modules folder?

If so, I think that will be impossible with yarn as yarn recognizes it is dirty and wipes away the changes. And with a future version of the package managers (like yarn 2) the node modules folder won’t exist. The files are read directly out of a zip file which wouldn’t be able to be modified.

@kmelmon
Copy link
Author

kmelmon commented Mar 24, 2020

Yes my initial thought was to copy all platform overrides into node_modules/react-native, as a postinstall step. Interestingly I'm using yarn today, and it seems to leave extra copied files around when running yarn install., but maybe my version/config is somehow different. I wasn't aware of yarn 2.

If we all agree with the idea that all platform overrides should live side-by-side with the files they are overriding, this problem of not being able to copy files into node_modules should be solvable. The simplest idea would be to copy everything from wherever the react-native JS files/out-of-tree JS overrides live to some well-known directory, and redirect react-native to this directory through the extraNodeModules prop. This would still allow us to have one metro config and one instance of metro for all platforms. Open to other ideas as well.

@kevinvangelder
Copy link

kevinvangelder commented Mar 30, 2020

I want to reiterate @cpojer's statement. Several of us have been supporting this approach for the past few years, and I'm really excited to see it gaining traction!

Let's figure out how to turn all platforms into out of tree platforms and support them first-class.

One of the most straightforward ways of implementing this would be to extract both iOS and Android from the React Native core and have them be installed as separate modules. This could be added as a question during the react-native init process, along the lines of "Which platforms do you wish to include? (iOS, Android, Windows, macOS)".

@cpojer
Copy link
Member

cpojer commented Mar 31, 2020

I think we need to split up iOS and Android into separate packages within the react-native package itself. RN already has a "packages" folder where this stuff could live, we'd just need to properly set up a monorepo (with lerna) and do the separation. Unfortunately with everything going on right now, I don't think anyone at Facebook will be able to prioritize this.

If you are interested in this work, I recommend you to make a plan on how we can split iOS and Android from react-native and list everything beyond just moving a few folders as well. After we have a plan, we can figure out how to tackle the work. A lot of it can likely be done via Pull Requests that we import and integrate into our internal repo.

cc @rickhanlonii who's been interested in this as well.

@NickGerleman
Copy link

NickGerleman commented Mar 31, 2020

Apart from just Android/iOS specific Javascript files, from what I've observed, shared JS is still often coupled to Android and iOS. We'll often see platform checks for Android and iOS, Android/iOS specific native module specs both included in the same file, etc.

More than separation into packages, making shared code truly platform agnostic would be a major undertaking. It's great long term for out of tree platforms (and nice for in tree), but also very expensive. I'm sure there will be instances where we can contribute to this while we're working to remove forked differences from RNW, but it feels like we would also want a shorter term solution while moving that direction.

@cpojer
Copy link
Member

cpojer commented Mar 31, 2020

@NickGerleman absolutely, that's a great point. We can start separating the files first, and then clean up each platform specific callsite in core to make sure any platform can be integrated easily.

@kmelmon
Copy link
Author

kmelmon commented Apr 13, 2020

Update: We now have two potential solutions for MSFT scenarios which we're debating:

Solution #1:
microsoft/react-native-windows#4522

With this solution, we create a react-native-installation directory within the app's project. An install script has the smarts to copy the react-native JS plus the overrides from the out-of-tree platform to this directory. In addition, metro.config.js and react-native.config.js is changed to point react-native to this installation directory.

Solution #2:
microsoft/react-native-windows#4576

With this solution, the out-of-tree platform continues to install itself with a copy of react-native into its own directory, like it does today. The change is to metro.config.js - a custom resolver is used to resolve all modules within react-native to the appropriate place, depending on the platform variable. Currently 'windows' is redirected to react-native-windows and all other platforms are redirected back to react-native.

There are pros and cons to these two approaches:
Solution #1 is more invasive to app developers as a script must run at install time to install everything to another directory. However it allows a single copy of react-native to be shared, rather than requiring each out-of-tree platform to ship with a copy of react-native.

Solution #2 is more scoped since no copying of files is done at install time. Instead it requires each out-of-tree platform to ship with a copy of react-native. This has the short-term advantage of working for react-native-macos with no further changes to it (to make react-native-macos work with solution #1, we'd need to create .macos files for every file that's been changed in the fork).

Both solutions end up in a place where a single metro.config can be used for all platforms, and one instance of metro can serve up bundles for all platforms.

Any thoughts/feedback would be appreciated!

@cpojer
Copy link
Member

cpojer commented Apr 14, 2020

I think #2 is a good solution for now.

@kmelmon
Copy link
Author

kmelmon commented Apr 14, 2020

Thanks for the feedback. We're going with solution 2.

@acoates-ms
Copy link
Collaborator

react-native-community/cli#1115 should implement this in a generic way in the CLI, so that any out of tree platform can get this functionality "for free"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🗣 Discussion This label identifies an ongoing discussion on a subject
Projects
None yet
Development

No branches or pull requests

9 participants