Skip to content

Conversation

@pepicrft
Copy link

@pepicrft pepicrft commented Feb 2, 2021

Summary:

At Shopify, we are using the React Native CLI in a monorepo setup. Since the CLI launches the packager taking the directory where React Native lives as the working directory, it causes the packager command to read the Metro configuration from the wrong directory.

This PR fixes it by giving the users the flexibility to customize the directory where the launch packager command and environment files are defined. On iOS, we changed the packager build phase to use our own launcher.

Please, let me know if you'd have taken a different approach. We only need to be able to specify where the Metro configuration lives.

Test Plan:

We tested it with our own monorepo and it works. We included that directory with the command and our own instructions on how to launch the packager.

cc @ElviraBurchik

@thymikee
Copy link
Member

thymikee commented Feb 2, 2021

This seems very specific to a certain edge-case use case. Can't we make sure the custom Metro config is propagated to the server instance? E.g. by utilizing Metro's --config flag, which would be more generic and flexible

@thymikee thymikee closed this Feb 2, 2021
@thymikee thymikee reopened this Feb 2, 2021
@pepicrft
Copy link
Author

pepicrft commented Feb 2, 2021

This seems very specific to a certain edge-case use case. Can't we make sure the custom Metro config is propagated to the server instance? E.g. by utilizing Metro's --config flag, which would be more generic and flexible

Sure! Do you think it's fine if I add a config argument to this list and then pass it to the launcher?

@thymikee
Copy link
Member

thymikee commented Feb 4, 2021

I think for now we can go with --metro-config and add it for iOS as well, for parity. What do you think @grabbou @satya164?

@pepicrft
Copy link
Author

pepicrft commented Feb 5, 2021

Sounds good. Unless there are objections, I'll go ahead and use that flag instead.

@grabbou
Copy link
Member

grabbou commented Mar 16, 2021

Thanks for the PR! I do agree with @thymikee that we should either utilise existing flags / Metro configuration instead of adding a new flag.

Right now, we obtain Metro configuration programatically here https://github.com/react-native-community/cli/blob/master/packages/cli/src/tools/loadMetroConfig.ts#L152 and we set cwd to root from Config. By design, Metro will look for a config there.

However, we also pass a second object, options, which - inside start and bundle commands, has historical config property ->

name: '--config [string]',
description: 'Path to the CLI configuration file',
parse: (val: string) => path.resolve(val),

You can use it to set a path to custom Metro configuration. The name probably needs to be updated, as historically, Metro/CLI config used to be a single file. While refactoring, we stopped relying on that property inside CLI, but kept it for Metro.

So, in general, I would say there are two ways going forward:

  • either add --config to missing commands
  • remove --config entirely and extend Config with metroConfig property. Users would have to create react-native.config.js file and define this property

While second option is more aligned with our long-term plan, I suspect it would break some tooling such as Expo. So as a short-term hack, I'd just stick to adding --config here and there.

One thing to consider on top of that would be to rename --config to --metro-config. While doing so, I would use Commander feature to define multiple names for an option and accept both occurrences. We don't want to break existing tooling, at the same time, we want to promote less confusing naming.

@pepicrft pepicrft closed this Mar 19, 2021
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.

3 participants