microsoft/rnx-kit

[Feature request] metro-config should add support for react-native-macos and react-native-windows

shirakaba opened this issue · 19 comments

What happened?

The docs suggest the following config:

// metro.config.js
const { makeMetroConfig } = require("@rnx-kit/metro-config");

module.exports = makeMetroConfig({
  transformer: {
    getTransformOptions: async () => ({
      transform: {
        experimentalImportSupport: false,
        inlineRequires: false,
      },
    }),
  },
});

This gives monorepo support, but it would be really helpful if it also made react-native-macos and react-native-windows "just work" by setting up whatever config they need, too.

Affected Package

@rnx-kit/metro-config

Version

0.6.8

Which platforms are you seeing this issue on?

  • Android
  • iOS
  • macOS
  • Windows
See system information

System Information

System:
  OS: macOS 14.5
  CPU: (16) arm64 Apple M3 Max
  Memory: 8.18 GB / 48.00 GB
  Shell:
    version: "5.9"
    path: /bin/zsh
Binaries:
  Node:
    version: 20.16.0
    path: ~/.volta/tools/image/node/20.16.0/bin/node
  Yarn:
    version: 1.22.19
    path: ~/.volta/tools/image/yarn/1.22.19/bin/yarn
  npm:
    version: 10.8.1
    path: ~/.volta/tools/image/node/20.16.0/bin/npm
  Watchman:
    version: 2024.06.10.00
    path: /opt/homebrew/bin/watchman
Managers:
  CocoaPods:
    version: 1.14.3
    path: /Users/jamie/.rbenv/shims/pod
SDKs:
  iOS SDK:
    Platforms:
      - DriverKit 23.5
      - iOS 17.5
      - macOS 14.5
      - tvOS 17.5
      - visionOS 1.2
      - watchOS 10.5
  Android SDK:
    API Levels:
      - "31"
      - "34"
    Build Tools:
      - 32.1.0
      - 33.0.0
      - 34.0.0
    Android NDK: Not Found
IDEs:
  Android Studio: 2021.2 AI-212.5712.43.2112.8609683
  Xcode:
    version: 15.4/15F31d
    path: /usr/bin/xcodebuild
Languages:
  Java:
    version: 17.0.12
    path: /usr/bin/javac
  Ruby:
    version: 2.6.10
    path: /Users/jamie/.rbenv/shims/ruby
npmPackages:
  "@react-native-community/cli": Not Found
  react: Not Found
  react-native: Not Found
  react-native-macos: Not Found
npmGlobalPackages:
  "*react-native*": Not Found
Android:
  hermesEnabled: true
  newArchEnabled: false
iOS:
  hermesEnabled: Not found
  newArchEnabled: false

Steps to Reproduce

  1. Set up a react-native project (whether in a monorepo or regular repo).
  2. Add react-native-macos and/or react-native-windows projects to it.
  3. Observe that Metro can't resolve files like node_modules/react-native-windows/Libraries/Image/Image.windows.js because it's still looking inside the react-native folder rather than substituting the react-native-windows folder when the platform is Windows.

Code of Conduct

  • I agree to follow this project's Code of Conduct

This is the config that is used in RNTA and it works on all platforms: https://github.com/microsoft/react-native-test-app/blob/trunk/example/metro.config.js

Can you share a repro where this doesn't work?

Think I’ll have my hands full for a few days but I’ll try to get back to you!

I think @shirakaba is trying to use it with the expo CLI, not the RN CLI. The RN CLI automatically handles adding the platform redirections (https://github.com/facebook/react-native/blob/main/packages/community-cli-plugin/src/utils/metroPlatformResolver.js), which maybe the expo CLI doesn't.

I wonder if this is something that should be resolved in expo CLI? Or should we detect expo CLI and do it here?

I wonder if this is something that should be resolved in expo CLI? Or should we detect expo CLI and do it here?

If the Expo CLI isn't, it probably should. I don't think we want to detect Expo CLI since we cannot determine which should be used in a mixed project (e.g. Expo for mobile and RN-CLI for desktop).

I wonder if we can merge them like below:

// metro.config.js
const { makeMetroConfig } = require("@rnx-kit/metro-config");
const { getDefaultConfig } = require('expo/metro-config');

const config = getDefaultConfig(__dirname);

module.exports = makeMetroConfig(config);

If this works, we can put it in the README.

Edit: Actually, if Expo has its own start command, maybe we can detect it anyway. Do we want to support this?

Good point, @acoates-ms! Yes, I'm using Expo CLI here. I hadn't expected that the RN CLI would implement that logic under-the-hood.

I think @tido64's solution of merging the configs is an elegant one.

I think explicitly merging the configs feels less magical (and therefore a lot better) than silently detecting whether we're running inside Expo CLI to run implicit logic.

If that would seem like the way to go, as well as putting it in the README, at some point we could create a reference third-party Expo template for iOS + Android + Windows + macOS whose metro.config.js looks like the above (see expo-template-bare-minimum for reference).

I'll try to test whether that config-merging approach works soon and get back to you.

Hmm, I tried that merging approach on an Expo-managed react-native-macos project and it's looking inside react-native instead of react-native-macos, which is the exact error I filed this issue about (albeit for react-native-windows):

Screenshot 2024-08-06 at 9 05 56

I'll try to dig into what's going wrong. I assume maybe just the merging has been applied in the wrong order.

Oh, right. We'll still need to merge in that metroPlatformResolver.js logic from RN CLI.

I got it working, but it's a bit gross.

const { makeMetroConfig } = require("@rnx-kit/metro-config");
const { getDefaultConfig } = require("expo/metro-config");

const defaultConfig = getDefaultConfig(__dirname);

/** @type {import("metro-config").MetroConfig} */
const config = {
  ...defaultConfig,
  resolver: {
    ...defaultConfig.resolver,
    // https://github.com/facebook/react-native/blob/4f47b385fa592942b34b3dba9f35bcc43677511b/packages/community-cli-plugin/src/utils/loadMetroConfig.js#L45
    resolveRequest: reactNativePlatformResolver({
      macos: "react-native-macos",
      windows: "react-native-windows",
    }),
  },
  serializer: {
    ...defaultConfig.serializer,
    // https://github.com/facebook/react-native/blob/4f47b385fa592942b34b3dba9f35bcc43677511b/packages/community-cli-plugin/src/utils/loadMetroConfig.js#L60C1-L73C9
    getModulesRunBeforeMainModule() {
      return [
        require.resolve("react-native/Libraries/Core/InitializeCore"),
        require.resolve("react-native-macos/Libraries/Core/InitializeCore"),
        require.resolve("react-native-windows/Libraries/Core/InitializeCore"),
        ...defaultConfig.serializer.getModulesRunBeforeMainModule(),
      ];
    },
  },
};

module.exports = makeMetroConfig(config);

/**
 * Copyright (c) Meta Platforms, Inc. and affiliates.
 *
 * This source code is licensed under the MIT license found in the
 * LICENSE file in the root directory of this source tree.
 *
 *
 * @format
 * @oncall react_native
 */

/**
 * This implementation is copy-pasted from the following file, which can't be
 * imported because it involves a subpath that's not allowed by the npm
 * package's exports map:
 * require("@react-native/community-cli-plugin/dist/utils/metroPlatformResolver.js")
 *
 * This is an implementation of a metro resolveRequest option which will remap
 * react-native imports to different npm packages based on the platform
 * requested. This allows a single metro instance/config to produce bundles for
 * multiple out-of-tree platforms at a time.
 *
 * @param platformImplementations
 * A map of platform to npm package that implements that platform
 *
 * Ex:
 * {
 *    windows: 'react-native-windows',
 *    macos: 'react-native-macos'
 * }
 */
function reactNativePlatformResolver(platformImplementations) {
  return (context, moduleName, platform) => {
    let modifiedModuleName = moduleName;
    if (platform != null && platformImplementations[platform]) {
      if (moduleName === "react-native") {
        modifiedModuleName = platformImplementations[platform];
      } else if (moduleName.startsWith("react-native/")) {
        modifiedModuleName = `${
          platformImplementations[platform]
        }/${modifiedModuleName.slice("react-native/".length)}`;
      }
    }
    return context.resolveRequest(context, modifiedModuleName, platform);
  };
}

It could get a bit cleaner by using loadMetroConfig to fill in the resolveRequest and serializer fields under-the-hood, but as that's an async function, I'm not sure that calling it is even an option inside metro.config.js.

Not sure where to go from here. Would duplicating that resolveRequest and getModulesRunBeforeMainModule config into @rnx-kit/metro-config be worth it just to clean up the user-side code? Should @rnx-kit/metro-config derive its config from @react-native/community-cli-plugin under-the-hood?

Or, back to @acoates-ms's opening thoughts:

I wonder if this is something that should be resolved in expo CLI? Or should we detect expo CLI and do it here?

I opened expo/expo#30820 on expo to see what they think. I don't think they are fundamentally opposed to the idea.

Not sure where to go from here. Would duplicating that resolveRequest and getModulesRunBeforeMainModule config into @rnx-kit/metro-config be worth it just to clean up the user-side code? Should @rnx-kit/metro-config derive its config from @react-native/community-cli-plugin under-the-hood?

This is exactly what @rnx-kit/metro-config already does:

defaultConfig.resolver.resolveRequest = outOfTreePlatformResolver(
availablePlatforms,
projectRoot
);
const preludeModules = getPreludeModules(availablePlatforms, projectRoot);
defaultConfig.serializer.getModulesRunBeforeMainModule = () => {
return preludeModules;
};

So it looks like it either doesn't work or something is overwriting it. This is why I would like to find out why it isn't working.

@shirakaba, can you try #3266 and let me know how it works in your repo?

For the benefit of other readers: we tried this in my repo and found that resolveRequest got deleted for some reason, so the final config wasn’t as intended. Needs further investigation.

For the benefit of other readers: we tried this in my repo and found that resolveRequest got deleted for some reason, so the final config wasn’t as intended. Needs further investigation.

And the current theory for this is that the project is missing @react-native/metro-config, which is required from 0.72, and would explain why resolveRequest is not set in the rnx's default config. And it would also make sense since Expo hides away the need.

Edit for more context: @rnx-kit/metro-config can't take a dependency on it because we don't want it to be tied to a specific version of React Native, but we can probably detect that it's missing and yell at you. Normally, this would be the job of @react-native-community/cli, but we were using expo start here.

Looking now, I see that @react-native/metro-config@0.73.5 is present in the root-level node_modules of my monorepo, just not listed as a direct dependency of my app. I think even if I did add it explicitly, pnpm would still hoist it to the monorepo root. What do you think?

I did a quick test to find out. I performed pnpm add -D @react-native/metro-config@~0.73.5 in my app directory. The dependency did indeed get hoisted, so it didn't show up in my app's node_modules yet, as before, it could be found in my monorepo root's node_modules.

This time, the custom platformResolver() instantiated inside outOfTreePlatformResolver() did get called, and bundling succeeded 🥳

So its mere presence in package.json seems to matter. Certainly, at the very least, yelling would be good. Would adding "@react-native/metro-config": "*" as a dependency or peerDependency be a better catch-all, though?

So its mere presence in package.json seems to matter. Certainly, at the very least, yelling would be good. Would adding "@react-native/metro-config": "*" as a dependency or peerDependency be a better catch-all, though?

It is already added as a peerDependency. I've pushed a change to check for RN version instead and throw an error if @react-native/metro-config cannot be found. Would really appreciate it if you could test the latest iteration as well before we merge.

Just copied the implementation from your commit let users know if @react-native/metro-config is missing.

Initial state

Just a quick regression test. With "@react-native/metro-config" still installed in my app's dependencies, and with your latest commit applied, I ran expo start again and got the usual warnings:

Starting Metro Bundler
The following packages should be updated for best compatibility with the installed expo version:
  @react-native-async-storage/async-storage@1.24.0 - expected version: 1.21.0
  expo@50.0.19 - expected version: ~50.0.20
  react-native@0.73.9 - expected version: 0.73.6
  react-native-svg@15.4.0 - expected version: 14.1.0
  @types/react@18.3.3 - expected version: ~18.2.45
Your project may not work correctly until you install the correct versions of the packages.

And, as I reported earlier when running your previous commit in the same conditions, the app ran fine; no red screen. So, no regressions.

With removal of @react-native/metro-config

This time, I uninstalled @react-native/metro-config from my app's dependencies (it's still available from the root of the monorepo's deps, of course) and ran expo start --clear to clear the Metro cache.

... however, the result was the same. Same warnings, no red screen, app just ran. It didn't warn me that I should install @react-native/metro-config. Instead, it "just worked". Was that the idea?

Just a heads-up that I noticed @byCedric recently self-assigned @acoates-ms's issue about this (expo/expo#30820), so we should just keep each other updated on whether Expo need do anything more here or should just leave it all to @rnx-kit/metro-config.

... however, the result was the same. Same warnings, no red screen, app just ran. It didn't warn me that I should install @react-native/metro-config. Instead, it "just worked". Was that the idea?

Yes, since @react-native/metro-config is still available from the root, it will use that copy. If you remove it entirely, you should see an error.

Just a heads-up that I noticed @byCedric recently self-assigned @acoates-ms's issue about this (expo/expo#30820), so we should just keep each other updated on whether Expo need do anything more here or should just leave it all to @rnx-kit/metro-config.

I think Expo will want to do address this anyway for users that don't want to or can't use @rnx-kit/metro-config.