Decouple Metro config from CLI config

[grabbou]

For context:

This is a continuation of what I originally started 2 years ago when we merged rnpm to react-native. It's been a long time, but better late then never. I am super excited this is finally getting long-deserved attention!

I have tried to separate this description into sections to make it easier to read and analyse. Please give it a review, accept or request changes.

In the meantime, I am going to back up from this PR for few days to clear up my mind. I spent last 25 hours coding this all night long like I was hypnotised. Never felt so good. I hope you share my excitement too after getting familiar with the diff.

Let's go!

Glossary

This PR touches some concepts and code that have been authored few years ago (oh wait, is React Native that old already? 🕵️‍♀️).

Please find the following table for explanation of some terms used later in this PR. Feel free to jump back to these definitions as you read along.

CLI - in this context - local-cli folder inside React Native repository. That's where the CLI lives. Each command (like link or start) is a separate folder with its own entry point.
Metro - the thing you use to bundle your app. In this context, it's mainly server and bundle folders inside local-cli that interface with it. It used to be called React Packager and was one of the first (if not the first one) piece to be moved out from React Native repository to a separate place
RNPM - a small tool I co-authored few years ago. It was meant to be an alternative to the CLI that was struggling with good UX and speed. We were asked to merge it to React Native and work along the React Native team on the improvements (what a coincidence now that we are doing the opposite hehe). Part of it was rnpm link command that later became react-native link.

Motivation

Historically, CLI was built around Metro (called React Packager back in the day). It's primary use-case was to serve a bundle for an app and then, bundle it with the production app. Since the whole CLI was Metro-centric, the CLI configuration was essentially a Metro configuration object. It was loaded from a separate file (usually rn-cli.config.js) and allowed to change the default config. It wasn't something publicly documented and definitely not something that should be used widely. Think of it more as an escape-hatch for advanced users that need that extra settings.

Part of the reason why React Native CLI was struggling was that there was no "standard" format for creating projects. There were libraries in the community having slightly different folder structure. Projects were being developed on their own. The CLI wasn't able to detect that and handle different formats. It was looking in hardcoded places for iOS files to run an app on the simulator. That was failing.

RNPM was different. It was smart enough to analyse the folder structure and normalise it into a JSON structure. That information was then stored in an object that became a "configuration object" for RNPM. This was super useful, because now, every command could've used normalised information about a project structure rather than doing the logic on its own. Each command, different logic. Bugs multiply. Another rationale was to enable 3rd party commands (or in other words, plugins) to be able to use the same mechanism. Think: you want to write some tools for your app. You rather focus on the tool itself, rather than finding where the iOS project is located (in this particular case).

When we were asked to merge RNPM to React Native and integrate it with the React Native CLI, I was super excited. The reason RNPM was started was to improve the overall quality of React Native tools and make it easier for everyone to get started with a new project.

We used the RNPM engine for commands (each command is a folder, we can load commands dynamically). Since configuration of RNPM and RN CLI (Metro config technically) were both objects, we ended up merging them together and injecting both at the same time to every command. We wanted to do the integration "as fast as possible" to not get delayed by any changes that are done to the React Native CLI in the meantime.

That resulted in a really annoying interface, part of it you can see in the diff.

(1) For example, since RNPM and Metro configuration were technically the same object, Metro was getting RNPM configuration too when initialised. I consider this a bad design - what if it overwrites something by a mistake?

(2) Another thing was strong-coupling with Metro, making it hard to welcome other bundlers to the React Native community and slowing down the start-up time. When you were running react-native run-ios, we would initialise the whole Metro configuration delaying the startup time. Without any need. The config was not even used. Just a property on a "config" object that was never accessed.

(3) And last thing - I don't think injecting "config" to every command was a right decision. Historically, we wanted to preserve a state. Say, you initialise a heavy configuration once and inject it to a command so that it can read it, instead of initialising it once again. Problem is - given the way CLI works - it usually runs a single command at a time. Even if it ever becomes an issue (say, we introduce chained commands), this shouldn't be big of a performance penalty, given the boilerplate it introduces.

This PR was something we were planning to do "as a follow-up", or "from the very beginning". Let's get to the details then.

Implementation

The diff looks relatively big, due to yarn.lock changes and few files moved around. See the following key concepts that are part of this PR:

(1) There's no "config" injected to a command. It is now called "context" and has "root" property - indicates where is the root of the project. This property is set by a --projectRoot flag. It is worth noting that this flag currently doesn't work in React Native master. Assuming that it works, it is still a Metro flag. On React Native master, this changes the root for the Metro, but React Native CLI (RNPM) still uses process.cwd(). That introduces ugly and annoying to debug features where your bundle is loaded, but there are no plugins and modules to link. This is especially annoying when working in a Yarn workspaces-bootstrapped environment. Not saying this fixes everything, but it's certainly a good step towards unification.

All methods that used to be available on "config", like "config.getProjectConfig" or "config.getPlatforms" are now pure functions that you can import and call. That was why this interface was in my opinion bad. Forcing everyone to call a function from a "config" object instead of just importing it gives us the assumption that there's some "state" involved. It's not.

(2) Metro config is now separate from CLI config and is located under utils/loadMetroConfig.js. Instead of being injected to every command as a 2nd argument, you can now manually initialise it by calling await loadMetroConfig(rootFolder, options). You can pass options to overwrite the configuration. Useful when you are calling from CLI (like we do) and want your flag to take precedence over all the other configurations (like the one present in a file).

(3) We also take advantage of "default" Metro configuration. If you are a React Native contributor and checked Flow errors once in a while, you remember "Metro configuration is not immutable" errors. This was because we were mutating a config, instead of passing the right defaults.

This resulted in a really annoying issues that started adding up over time.

Let me show you an example (from React Native 0.57, tested on a same project as this branch):

The default Metro configuration specifies the following "platforms" and "providesNodeModules":

platforms: [ 'ios', 'android', 'windows', 'web' ],
providesModuleNodeModules: [ 'react-native', 'react-native-windows'],

Later, in React Native 0.57's CLI, we have the following line:

config.resolver.platforms = config.resolver.platforms
    ? config.resolver.platforms.concat(defaultConfig.getPlatforms())
    : defaultConfig.getPlatforms();

Essentially, it appends "our platforms" (as defined in React Native CLI here) to the already resolved platforms, when present.

Because config.resolver.platforms is always defined (checked Flowtype inside Metro), our default platforms will always get appended.

This results in the following "platforms" and "providesModuleNodeModules" values, as console logged in React Native 0.57:

platforms: [ 'ios', 'android', 'windows', 'web', 'ios', 'android', 'native' ],
providesModuleNodeModules: [ 'react-native', 'react-native-windows', 'react-native' ],

Issues?

Quite a few:

• windows is there but I don't have react-native-windows integrated
• web is before native (if I ever happen to have a.web.js and a.native.js, the former will get resolved which can be a source of really annoying issues)
• react-native-windows is there but I don't have react-native-windows in my project at all.

What happens when I really add react-native-windows?

platforms:
      [ 'ios',
        'android',
        'windows',
        'web',
        'ios',
        'android',
        'native',
        'windows' ],
     providesModuleNodeModules:
      [ 'react-native',
        'react-native-windows',
        'react-native',
        'react-native-windows' ],

Issues? Same as previously, but also: react-native-windows defined multiple times. windows defined after native.

I wasn't unable to figure it out with @rafeca as why it could be happening, but my assumption is that there used to be no defaults in Metro back in the day. They got added to support React Native Windows use-case (in form of a hardcoded path). Then, later, someone added dynamic support for new platforms in React Native, which also counts for React Native Windows hence the duplication.

This is example of a really nasty bug we can get rid of automatically when we decouple configs from each other and stick to a clean public API.

In contrast, this is the configuration that this branch produces, with react-native-windows present:

platforms: [ 'ios', 'android', 'native', 'windows' ],
providesModuleNodeModules: [ 'react-native', 'react-native-windows' ],

It still has windows after native (which I consider a glitch, because .native.js should also work for windows platform). I don't want to change this behaviour in this PR, so I left an appropriate todo comment.

And here's the config without react-native-windows being part of a project:

platforms: [ 'ios', 'android', 'native' ],
providesModuleNodeModules: [ 'react-native'],

That makes much more sense to me now!

(4) Flowtypes everywhere! This library uses them quite heavily, so just ended up adding few more to make this refactor a bit more convenient to me.

(5) No changes to the link or unlink or all the commands. Just changed the first file where it loads configuration to support new format. Then, tests were made sure to pass.

Breaking changes

(1) There's no config.xxx() available. You should import a pure function from a "core" module instead.

(2) There's no Metro config available under config. If you were reading Metro properties, like config.resolver.platforms, you should figure out your way out of it. Alternatively, init Metro config manually by running loadMetroConfig(pathToRoot, options). There's nothing we can do about this change. I don't think it affects anyone as it was primarily a side-effect.

What's next?

Before this PR gets merged, I would like to:

(1) test it with those who use different platforms. Please let me know in comments you'd like to help!

(2) test it with those who use different configuration objects. Please let me know if you'd like to help!

You know, tests are passing, but we need to check for some uncovered pieces. I don't expect issues to be major due to Flowtypes and test coverage, but it's good to minimise them before sending out to users for testing.

Once this PR gets merged, I would like to:

(1) make packages/metro-tools folder here, release it as @react-native/cli-metro-tools (or similar) and consume Metro just like any other bundler. This opens a road for inviting other bundlers to the table in the future. It also makes it much easier to upgrade CLI along new Metro releases and test it. We can also work closely with the Metro team on that particular piece of the CLI only.

(2) make packages/cli-core folder and release as @react-native/cli-tools (or similar) where we will publish all the code that was present under "config" object in the past. So instead of running "config.getProjectConfig" and "react-native-cli/core/getProjectConfig", you can just "import { getProjectConfig" from "react-native/cli-tools" and stick to the public interface. That makes it possible for us to follow server and keep offering a nice upgrade path for everyone.

==========

That's it. If you are still reading here - good luck, you made it through the biggest description of a PR in my life. 😂

Thank you for your time - looking forward to your comments!

[kelset]

cc @rozele

[grabbou]

How to try it out

First, install the package to a relatively recent React Native project:

cd YourReactNativeProject
yarn add react-native-local-cli-preview

CLI depends on Metro 0.50.x (currently 0.57 and 0.58 depend on older versions) and there might be some issues because of that, let me know in case you need a different setup.

Then, run:

node ./node_modules/react-native-local-cli-preview/cli.js --help

You can't use yarn react-native because that will go to react-native CLI instead.

Note: Make sure to remove this project once you are done testing. Not recommended to use it that way for now. We will be shipping this package with every React Native version to make sure Metro dependency (and others) are correct.

[cpojer]

yo, your prettier config should not have these whitespaces.

[grabbou]

Well, we don't have prettier set up in the repository just yet. I was waiting till we finish "migration" with that and then, I started this PR.

I was planning to use our own preset https://github.com/callstack/eslint-config-callstack as it already has prettier integration built into ESLint rules (it automatically configures itself based on ESLint settings).

That way we only need to run eslint --fix.

I would suggest to "temporarily" ignore styling issues (as long as they are not that "annoying") and I'll follow up with "prettier" PR right after this one is merged.

[grabbou]

Update: I didn't find many people that have written a plugin before hence I don't think many would be affected by a breaking change.

Anyway, I decided to provide a legacy interface + print deprecation warnings instead of breaking to make it easier for some developers to gradually upgrade. I think if we can avoid a breaking change without much extra cost, we should do that.

[grabbou]

There are two bugs that we've noticed during testing - I am going to follow up either later today or tomorrow with the final round up of fixes.

Meanwhile, please keep testing if you have a spare minute.

[satya164]

Maybe instead of (val: string) => (val === 'false' ? false : true), you could do !!JSON.parse(val)

[satya164]

Maybe instead of loading all commands upfront, you could load them as needed? e.g. look one by one, stop looking for more when the command being used is found. Might improve startup time.

[grabbou]

Good catch. We should improve that in the next PR. Noted.

[kelset]

Left a couple comments about headers & copyright, but aside from that looks solid. Can't wait for it be integral part of the DX