[@react-native/community-cli-plugin] Added inference of watchFolders from NPM, Yarn and PNPM workspaces

[kraenhansen]

Note

This depends on the merge and release of facebook/metro#1206

Summary:

When serving the Metro server from a mono-repo leveraging NPM or Yarn workspaces, the developer need to manually configure Metro to include the workspace root in the watchFolders (or projectRoot).

I'm aware the team might already be working on a solution for this, but I wanted to suggest this in case you hadn't started work on it or hadn't considered this approach.

Resolving the workspace root

I initially experimented with a solution using @npmcli/arborist to resolve the workspace root, but I stopped and implemented the getWorkspaceRoot function instead as soon as I realised the package only provided async APIs to load the "actual tree".

This PR suggests adding a getWorkspaceRoot function to resolve the workspace root, which supports NPM and Yarn workspaces. It use the same dependency to resolve globs (micromatch) which may occur in Yarn workspaces. I've double-checked that neither NPM nor Yarn supports workspaces outside of descendent folders, i.e. the root will always be in a parent directory of a package.

I've considered using existing packages to find the workspace root, but I wanted a solution that would be robust to malformed package.json files and thought the algorithm was simple enough to inline here. If the reviewer thinks this is not important enough, I can update the PR to use the find-yarn-workspace-root, which seem to be a fairly safe dependency to take on (with 2 mill. weekly downloads).

watchFolders vs projectRoot

The Metro config suggests using the projectRoot when configuring for a mono-repo:

If your Metro project is developed in a monorepo and includes files from multiple logical packages, you'll generally want to set projectRoot to the root of your repository, or at least high enough in the hierarchy that all relevant files are reachable without separately configuring watchFolders.

When you do this, the ./index entry-point is expected to be in the workspace root, which is definitely not desirable. I wasn't able to find a way to configure this back to using the original projectRoot passed by the user and found it would be simpler to just use the watchFolders for this use-case. I'd love some feedback on this decision.

Changelog:

[GENERAL] [ADDED] - Add a default watchFolders Metro config, automatically locating the root of NPM, Yarn and PNPM workspaces.

Test Plan:

I've tested this in an app locally and I've added a few tests to verify the implementation of getWorkspaceRoot.

[analysis-bot]

Platform	Engine	Arch	Size (bytes)	Diff
android	hermes	arm64-v8a	17,159,181	-38,174
android	hermes	armeabi-v7a	n/a	--
android	hermes	x86	n/a	--
android	hermes	x86_64	n/a	--
android	jsc	arm64-v8a	20,538,555	-37,684
android	jsc	armeabi-v7a	n/a	--
android	jsc	x86	n/a	--
android	jsc	x86_64	n/a	--

Base commit: 07e8ae4
Branch: main

[robhogan]

Hey @kraenhansen - love this in general as a pragmatic (but careful) default heuristic, thanks for the contribution.

The Metro config suggests using the projectRoot when configuring for a mono-repo: [...] I wasn't able to find a way to configure [the app entry point] back to using the original projectRoot passed by the user and found it would be simpler to just use the watchFolders for this use-case. I'd love some feedback on this decision.

I agree (see facebook/metro#1016), - both approaches are valid and I don't think the advice in the Metro docs is the path of least resistance for typical single app Metro instances (the tradeoffs are different for running Metro standalone, serving multiple apps), but we haven't had chance to update those docs.

thought the algorithm was simple enough to inline here

👍

NPM / Yarn workspaces root

My main hesitation with this PR is introducing a split between support for Yarn/NPM workspaces vs pnpm workspaces - was this something you looked into? One possible complication is that if pnpm also supports nested roots we'd need to add the complication of Yaml parsing, but glob matching should be the same. I wouldn't say pnpm support is a blocker though.

[robhogan]

Can we avoid globbing the file system here? We should only need to check that projectRoot satisfies one of the globs, which we can do without IO - we don't need to enumerate the other matches on disk.

[kraenhansen]

I agree, this could be refactored to be less IO intensive. I'll update this to use micromatch (which is the underlying dependency of fast-glob)

[robhogan]

Hmm.. this is awkward, because if there is an error here for whatever reason (less chance if we check for permissions above), even if the user explicitly specifies watchFolders, they can't (in this design) avoid running this auto-detection, generating warnings that aren't relevant to them.

Ideally, this detection would only run if the user hasn't specified watchFolders themselves, but to do that I think we'd need to move it to the @react-native/community-cli-plugin - and tbh that might be a better place for it than amongst these static defaults. (WDYT @huntie ?)

[huntie]

I think that sounds fair 👍🏻

Expo does perform similar detection as part of its config defaults, however that is (conceptually) downstream from this package. I'd rather locate config that is dependent on the embedder (Community CLI, Expo), particularly if dynamic, outside this file. So yeah, into @react-native/community-cli-plugin.

[huntie]

I think that sounds fair 👍🏻

Expo does perform similar detection as part of its config defaults, however that is (conceptually) downstream from this package. I'd rather locate config that is dependent on the embedder (Community CLI, Expo), particularly if dynamic, outside this file. So yeah, into @react-native/community-cli-plugin.

[kraenhansen]

I moved the implementation into the community-cli-plugin package and updated the changelog entry. If you're happy with the approach, I could continue by adding support for pnpm workspaces too (taking on a dependency of a yaml parser) or we could merge this and I'd be happy to do a follow-up PR for that.

@robhogan / @huntie: Do you have any thoughts on the use of watchFolders vs projectRoot as I've mentioned in the description of the PR.

[robhogan]

Thanks again @kraenhansen! I'm happy for pnpm to be a follow-up.

Re watchFolders vs projectRoot, I mentioned that a bit above (more in the linked issue), but tldr I agree that customising watchFolders is the right approach for the RN template default.

[kraenhansen]

The only potential problem I see with this approach of providing the default as an override after loading the user-land config, is that the developer has to provide at least one element (other than the project root which is automatically injected) in the watchFolders to disable this default heuristic. I.e. it's not enough to simply pass an empty array from the user's config.

[huntie]

Yeah it's tricky 😅.

I think let's bypass this logic if config.watchFolders != null. i.e. any value set by the user is an opt-out. We'd also want to note this in the docs — under a header "Workspaces detection" or similar (which could also briefly describe this feature to users 🙂).

[kraenhansen]

config.watchFolders != null

One problem with that is that loadConfig from metro-config injects the project root as watch folder: https://github.com/facebook/metro/blob/main/packages/metro-config/src/loadConfig.js#L325 .. so I don't think a user will ever be able to configure this as null.

Thinking about this more, I wonder if it'd be better to use the defaultConfigOverrides argument passed to loadConfig to implement this 🤔 I'll experiment a bit with this.

[huntie]

Thinking about this more, I wonder if it'd be better to use the defaultConfigOverrides argument passed to loadConfig to implement this 🤔

Yes I think that would work :). FYI the current result of getOverrideConfig must be assigned last via mergeConfig (always overriding) — the case of watchFolders is distinct from this. Therefore we likely want another function in this file, e.g. getDynamicDefaultConfig — with a comment mentioning that this is assigned to defaultConfigOverrides and each value is overridable by the project (again distinct from this function).

[kraenhansen]

I did some experiments passing the workspace root via the second parameter to metro-config's loadConfig function.

It turns out that even if we omitted watchFolders from the config here:

react-native/packages/metro-config/index.js

Line 85 in f1a7f08

watchFolders: [],

it would still propagate as [] to the community-cli-plugin because the metro-config package as watchFolders: [] here: https://github.com/facebook/metro/blob/main/packages/metro-config/src/defaults/index.js#L163 and we merge on top of that:

react-native/packages/metro-config/index.js

Lines 91 to 94 in f1a7f08

	return mergeConfig(
	getBaseConfig.getDefaultValues(projectRoot),
	config,
	);

I tried explicitly setting the watchFolders to undefined in @react-native/metro-config, but metro-config's loadConfig expects this to be iterable: https://github.com/facebook/metro/blob/main/packages/metro-config/src/loadConfig.js#L326 🤔

The only way I can get it working is by deleting the watchFolders property on the object returned from getDefaultValues here:

react-native/packages/metro-config/index.js

Line 92 in f1a7f08

getBaseConfig.getDefaultValues(projectRoot),

while omitting the watchFolders from the config in @react-native/metro-config entirely. Only issue is that the object returned from getDefaultValues is $ReadOnly.

I've pushed another branch to illustrate the changes we'll need: kraenhansen/react-native@kh/metro-config-workspace-root...kraenhansen:react-native:kh/metro-config-workspace-root-defaults

[huntie]

metro-config's loadConfig expects this to be iterable: facebook/metro@main/packages/metro-config/src/loadConfig.js#L326

Interesting! I think we should fix that in Metro as watchFolders can be undefined as indicated in InputConfigT.

---

Separately, and back to the problem at hand (forgive me if I've lost context on something obvious) — can we instead:

• Keep the default of watchFolders = [].
• Apply the heuristic if watchFolders is [] or == null, and do nothing otherwise.
  • What's relevant to the user is have they set an override for watchFolders in their metro.config.js? (i.e. in the template). If not, then we can apply this :)
  • This would be documented as ~"watchFolders behaviour and workspaces detection: The default for watchFolders is []. If you override this with a custom set of paths (i.e. a nonempty array), workspaces detection will not be applied."

[kraenhansen]

Okay. I misunderstood you then. I thought you wanted end-users to be able to use an empty array to indicate that they want to opt out of the workspace detection.

The metro-config's loadConfig injects the project root as a watch folder, so from the cli plugin, it won't ever be empty, but instead I suggested to check if it contains any other value than the project root. (The line this thread refers to).

[robhogan]

Hmm - a user may explicitly specify watchFolders: [] and in that case we should not override it by applying auto-detection. A referential check could work (to differentiate explicit [] from the default []), though not the clearest API.

(IMO, this would be easier if metro.config.js exported a partial InputConfigT, as it used to, rather than a full ConfigT with defaults already merged and explicit intent lost, as it does post @react-native/metro-config. I don't fully recall why we did it that way @huntie? I think it was discussed and I'm just forgetting the trade-offs.)

[huntie]

Hmm. That default config merge is applied inside @react-native/metro-config, so we have control. The partial config was our initial approach, pivoted away from here:

• Restore base config merge in metro-config #38092

In short, it was the perceived UX benefit of being able to extend nonempty defaults from @react-native/metro-config, without separately requiring metro-config in an RN project.

Now that we've documented Metro config in RN better, perhaps we could move forward with the following changes in @react-native/metro-config:

• Removing the default config merge.
• Removing watchFolders: [] in these defaults.
• Artificially reading and assigning the defaults for sourceExts and assetExts (to fit user expectations as this is frequently extended from).

[kraenhansen]

I finally found some time to get back to this:

• I've implemented a couple of tests, including a failing test for the case where a developer explicitly set their watchFolder: [] in the metro.config.js.
• I've realised, that even if we fix the bug with metro-config not taking watchFolders: undefined, I would have to move out the injection of configWithArgs.projectRoot over to community-cli-plugin (to determine if the developer left out the value or explicitly set it to the empty array) .. but that could break someone relying on metro-config injecting this. I suggest adding an optional argument to metro-config's loadConfig to disable this, while limiting breakage of anyone else depending on it.

I've create a PR to metro to illustrate the change (facebook/metro#1206). I'm copying the entire overriddenConfig part into the community-cli-plugin's overrides.

[kraenhansen]

I pushed support for an alternative way of declaring workspaces in Yarn.

[kraenhansen]

I also got around to adding support for PNPM workspaces and I choose to add a dependency on yaml, since that was already used by @react-native-community/cli-doctor and in the yarn.lock ... now, I'll let the dust settle for you to properly review 😅

[huntie]

Yeah it's tricky 😅.

I think let's bypass this logic if config.watchFolders != null. i.e. any value set by the user is an opt-out. We'd also want to note this in the docs — under a header "Workspaces detection" or similar (which could also briefly describe this feature to users 🙂).

[kraenhansen]

The tests pass locally if I patch metro-config by deleting the injection of the project root (see my comment above) and instead return configWithArgs early.

The injection of projectRoot as watchFolder is still happening, but now it has been moved into the community-cli-plugin:

react-native/packages/community-cli-plugin/src/utils/loadMetroConfig.js

Lines 57 to 58 in 7fd9661

	// Always include the project root as a watch folder, since Metro expects this
	const watchFolders = [config.projectRoot];

[kraenhansen]

This would need an update once facebook/metro#1206 is merged, released and pulled in.

Suggested change

	const config = await loadConfig(
	{
	cwd,
	...options,
	},
	{
	// Enables users to explicitly specify watchFolders
	watchFolders: undefined,
	},
	);
	const config = await loadConfig(
	{
	cwd,
	...options,
	},
	{
	// Enables users to explicitly specify watchFolders
	watchFolders: undefined,
	},
	false
	);

[react-native-bot]

This PR is stale because it has been open 180 days with no activity. Remove stale label or comment or this will be closed in 7 days.

[kraenhansen]

Please keep this open, I'll rebase as I return from vacation🤞