fix(metro-service): pass asset plugins on to Metro

[Bao Nguyen (giaBaoJS)]

Description

loadMetroConfig throws as soon as a project configures asset plugins.

rnx-kit/packages/metro-service/src/config.ts

Lines 216 to 219 in 84d6e47

	if (overrides.assetPlugins) {
	// @ts-expect-error We want to assign to read-only `assetPlugins`
	defaultConfig.transformer.assetPlugins = assetPlugins;
	}

  if (overrides.assetPlugins) {
    // @ts-expect-error We want to assign to read-only `assetPlugins`
    defaultConfig.transformer.assetPlugins = assetPlugins;
  }

There are two problems on that one line:

1. assetPlugins is not declared anywhere in config.ts. The only binding in
   scope is the overrides parameter, whose type declares assetPlugins?: string[]
   at config.ts:21.
   Reading the free identifier throws ReferenceError: assetPlugins is not defined.
   The @ts-expect-error above it suppresses the whole line, so the TS2304
   that would have caught this never surfaced (the directive only claims to be
   silencing the read-only assignment, TS2540).
2. Replacing it with overrides.assetPlugins is still not enough. On React
   Native 0.72 and above, getDefaultConfigProvider returns () => ({})
   because @react-native/metro-config resolves, so defaultConfig.transformer
   is undefined and the assignment throws
   TypeError: Cannot set properties of undefined (setting 'assetPlugins').

This builds the input config handed to loadConfig instead of mutating a
possibly absent nested object, and keeps whatever transformer options the
default config provider did return. The @ts-expect-error is no longer needed
because nothing read-only is written to.

Reachable from rnx-cli start: serve/kit-config.ts:45
merges assetPlugins into the server config, and start.ts:76-82
passes them into loadMetroConfig. So rnx-cli start --asset-plugins <list>,
or a rnx-kit.server.assetPlugins entry in package.json, hits it.

Note that assetPlugins is not part of Metro's YargArguments, so spreading
overrides into loadConfig does not carry it; the explicit assignment is
still needed.

Test plan

loadMetroConfig takes the default config provider as an optional third
parameter (/** @internal */ getDefaultConfig), so tests can supply one
directly instead of needing a fixture per default config shape.

packages/metro-service/test/config.test.ts covers both shapes:

• an empty default config, as returned on React Native 0.72 and above, where
  transformer is absent.
• a populated transformer, as returned by
  @react-native-community/cli-plugin-metro.

One fixture under test/__fixtures__/metro-config/ remains, stubbing
metro-config so the test can inspect the config that loadConfig was handed.

With the fix (packages/metro-service):

$ yarn test
▶ loadMetroConfig
  ✔ passes asset plugins on to Metro when there is no default config (1.563125ms)
  ✔ passes asset plugins on to Metro without dropping transformer options (0.275834ms)
✔ loadMetroConfig (2.4745ms)
ℹ tests 25
ℹ pass 25
ℹ fail 0

Reverting only the body of loadMetroConfig to its state on main, both go
red for the right reason:

✖ passes asset plugins on to Metro when there is no default config
  ReferenceError: assetPlugins is not defined
      at loadMetroConfig (.../packages/metro-service/src/config.ts:218:46)

✖ passes asset plugins on to Metro without dropping transformer options
  ReferenceError: assetPlugins is not defined
      at loadMetroConfig (.../packages/metro-service/src/config.ts:218:46)

Applying only the one-word fix (assetPlugins to overrides.assetPlugins,
directive kept) shows the second problem in isolation:

✔ passes asset plugins on to Metro without dropping transformer options
✖ passes asset plugins on to Metro when there is no default config
  TypeError: Cannot set properties of undefined (setting 'assetPlugins')
      at loadMetroConfig (.../packages/metro-service/src/config.ts:218:44)

yarn build, yarn test, yarn lint and yarn format are clean in
packages/metro-service.

[Tommy Nguyen (tido64)]

Thanks for fixing this. Do you think we can pass in the getDefaultConfig function in tests instead? We can change the loadMetroConfig signature like below:

diff --git a/packages/metro-service/src/config.ts b/packages/metro-service/src/config.ts
index dead5d1b0..7faa99402 100644
--- a/packages/metro-service/src/config.ts
+++ b/packages/metro-service/src/config.ts
@@ -208,9 +208,9 @@ function getDefaultConfigProvider(
  */
 export function loadMetroConfig(
   cliConfig: CLIConfig,
-  overrides: MetroConfigOverrides
+  overrides: MetroConfigOverrides,
+  /** @internal */ getDefaultConfig = getDefaultConfigProvider(cliConfig.root)
 ): Promise<ConfigT> {
-  const getDefaultConfig = getDefaultConfigProvider(cliConfig.root);
   const defaultConfig = getDefaultConfig(cliConfig);

   if (overrides.assetPlugins) {

This way we don't have to create fixtures for every edge case.

[Bao Nguyen (giaBaoJS)]

Done, pushed. loadMetroConfig now takes getDefaultConfig as an optional third parameter defaulting to getDefaultConfigProvider(cliConfig.root), and the tests pass one in.

The two per default config fixtures are gone. One fixture remains because loadConfig is still resolved with requireModuleFromMetro("metro-config", cliConfig.root), so the test root needs a resolvable react-native and metro-config to stub out. Happy to inject that one too if you prefer.

Trade off worth flagging: getDefaultConfigProvider is no longer exercised by these tests, they now cover only the config the provider's output is merged into.