kristerkari / react-native-svg-transformer

Import SVG files in your React Native project the same way that you would in a Web application.
MIT License
1.58k stars 115 forks source link

Unexpected import of expo metro config #333

Closed Titozzz closed 2 months ago

Titozzz commented 8 months ago

We use expo libraries without expo-cli. Theses lines of code will use the wrong transformer based only on the fact that expo metro config exists in node_modules.

/**
 * `metro-react-native-babel-transformer` has recently been migrated to the React Native
 * repository and published under the `@react-native/metro-babel-transformer` name.
 * The new package is default on `react-native` >= 0.73.0, so we need to conditionally load it.
 *
 * Additionally, Expo v50.0.0 has begun using @expo/metro-config/babel-transformer as its upstream transformer.
 * To avoid breaking projects, we should prioritze that package if it is available.
 */
const upstreamTransformer = (() => {
  try {
    return require("@expo/metro-config/babel-transformer");
  } catch (error) {
    try {
      return require("@react-native/metro-babel-transformer");
    } catch (error) {
      return require("metro-react-native-babel-transformer");
    }
  }
})();

You shouldn't assume the transformer based on dependencies, so I'd suggest an extra param / option! What do you think 😃 ?

kristerkari commented 8 months ago

Previously there was always only one Babel transformer per project, but now that Expo introduced its own transformer the code is just expecting there to be either Expo or React Native transformer, not both.

Adding an option to override the transformer path would be one way to handle this problem.

kristerkari commented 7 months ago

I've been thinking that the easiest fix for now might be just to add more checks to try to verify that the app is a real Expo app, and not just a project that happens to have some expo dependencies installed.

ravindran-a commented 6 months ago

+1

EvanBacon commented 6 months ago

Yes, the Metro config isn't very good at plugins and mostly just offers an API for forking different aspects of the bundling pipeline, i.e. changing the transformer rather than appending rules to a standard transformer.

We've documented that cases like SVG should be handled inside a custom transformer in the user project https://docs.expo.dev/versions/latest/config/metro/#extending-the-babel-transformer

A more ideal API would look like rollup or webpack where you indicate matchers and functions for handling a file when matched.

For some background, Metro has multiple layers of transformation (transformerPath, transformer.babelTransformerPath, project babel config), all of which pull in some amount of Babel, effectively making them mostly share the same responsibilities. Ideally there would just be a transformPath and babel config, with transform plugins that get pulled in via the transformer implementation in the transformPath.

In Expo CLI, we're moving to simplify the Metro bundler behavior. We've moved string-based transformation to the transformWorker, and most Babel work to the babel-preset-expo package. The babelTransformerPath simply loads the babel config and passes Metro options to the babel caller for use in babel-preset-expo. This is in contrast to the community version which injects the react-refresh plugin (we do this in babel-preset-expo) and adds additional settings to the babel preset when a local config doesn't exist (also done in babel-preset-expo).

kristerkari commented 2 months ago

Regarding React Native projects that happen to use some Expo dependencies, I have opened a PR #366 to add support for defining the transformer based on the project:

-require.resolve("react-native-svg-transformer")
+require.resolve("react-native-svg-transformer/react-native")

This is only additional config, and won't need migration for existing projects.

Please let me know if you think that this is good appoarch to work around the problem.

Titozzz commented 2 months ago

@kristerkari this PR is 💯 . Thanks for the idea

kristerkari commented 2 months ago

@Titozzz thanks for reporting the problem, and please let me know if you run into any problems with the newest version. 👍