Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

sourceExts and assetExts missing from resolver #1010

Closed
tiagomsmagalhaes opened this issue Jun 22, 2023 · 9 comments
Closed

sourceExts and assetExts missing from resolver #1010

tiagomsmagalhaes opened this issue Jun 22, 2023 · 9 comments

Comments

@tiagomsmagalhaes
Copy link

tiagomsmagalhaes commented Jun 22, 2023

Do you want to request a feature or report a bug?

A bug

What is the current behavior?

sourceExts and assetExts are missing from getDefaultConfig(__dirname).resolver. I'm using them to parse scss and svg files like such

module.exports = (async () => {
  const {
    resolver: {sourceExts, assetExts},
  } = await getDefaultConfig();
  return {
    transformer: {
      getTransformOptions: async () => ({
        transform: {
          experimentalImportSupport: false,
          inlineRequires: false,
        },
      }),
      babelTransformerPath: require.resolve('./rn-transformer.js'),
    },
    resolver: {
      assetExts: assetExts.filter(ext => ext !== 'svg'),
      sourceExts: [...sourceExts, 'scss', 'css', 'pcss', 'sass', 'svg'],
    },
  };
})();

If the current behavior is a bug, please provide the steps to reproduce and a minimal repository on GitHub that we can yarn install and yarn test.

Repo

yarn install && yarn start

What is the expected behavior?

Currently sourceExts and assetExts are missing from getDefaultConfig(__dirname).resolver as it would on the log after starting the project

Please provide your exact Metro configuration and mention your Metro, node, yarn/npm version and operating system.

node v16.17.1
yarn version v1.22.19
Ventura 13.4
@huntie
Copy link
Member

huntie commented Jun 22, 2023

Can you confirm which getDefaultConfig function you are calling?

  • metro-config provides the defaults for all Metro projects (including sourceExts and assetExts).
  • @react-native/metro-config provides the additional defaults for React Native — and will not necessarily customise these values.

@tiagomsmagalhaes
Copy link
Author

The repo above is as vanilla it can be, using @react-native/metro-config. I faced this issue when trying to upgrade react-native from 0.71 on 0.72 on an existing project I have with the help of the upgrade helper (although 0.72 is not available yet on the helper).

I did all the changes then tried to adapt the snippet above into the default config, then I started getting issues and consequently started to debug the issue. I also couldn't find a mention of things moving around (possibly overlooked), hence the creation of this issue.

@tiagomsmagalhaes
Copy link
Author

I tried to replace @react-native/metro-config with metro-config but cannot use getDefaultConfig

But I've checked that sourceExts and assetExts are there in the metro-config package

@huntie
Copy link
Member

huntie commented Jun 22, 2023

I tried to replace @react-native/metro-config with metro-config but cannot use getDefaultConfig

This should work, via await getDefaultConfig(__dirname).

Recommended alternative

If you're customising sourceExts/assetExts only, these can be referenced from metro-config or copied into your config directly (source) — since these do not change often and it's nice to have the source of truth within your project.

const {assetExts, sourceExts} = require('metro-config/defaults/defaults');

Advanced alternative
For power users that need dynamic customisation

You should also be able to export a function that will provide access to Metro's base config (matching the installed version of Metro). Please note that you must still merge this with @react-native/metro-config's defaults (as shown, defensively merged up front).

const {getDefaultConfig, mergeConfig} = require('@react-native/metro-config');

module.exports = function (baseConfig) {
  const defaultConfig = mergeConfig(baseConfig, getDefaultConfig(__dirname));
  const {resolver: {assetExts, sourceExts}} = defaultConfig;

  return mergeConfig(
    defaultConfig,
    {
      resolver: {
        assetExts: assetExts.filter(ext => ext !== 'svg'),
        sourceExts: [...sourceExts, 'scss', 'css', 'svg'],
      },
    }
  );
};

Separately, I think this is a gotcha with facebook/react-native#36777, particularly as we named the main function getDefaultConfig. It is most likely more ergonomic to have @react-native/metro-config provide the complete, extended-from-defaults config object (the original setup prior to that PR, and matching Expo). cc @motiz88 @robhogan @kelset

  • ➡️ I'm going to briefly document the above options either in @react-native/metro-config's README or our docs.
  • We should consider reviewing Metro's overall config loading approach (particularly ahead of an extends option).

@huntie
Copy link
Member

huntie commented Jun 27, 2023

Noting facebook/react-native#38069 as another datapoint for the user expectation that @react-native/metro-config replaces config exports from metro-config 😐. cc @motiz88

I think there's reasonable merit to switch (back) to merging the base config within @react-native/metro-config, for convenience. This would match the precedent of @expo/metro-config and @rnx-kit/metro-config, which each export complete configs.

This is counterweighted by the correctness win of our current approach: by specifying the minimum additional config for React Native, we inherit the underlying defaults from Metro that match the current Metro version.

(@kelset this might be an add-on we want to include for 0.72.1)

@kelset
Copy link
Contributor

kelset commented Jun 27, 2023

just a quick headsup, current ETA for 0.72.1 is this Thursday; if a commit about this won't land before that, it will have to wait for 0.72.2

huntie added a commit to huntie/react-native that referenced this issue Jun 27, 2023
Summary:
Reverts facebook#36777.

This is motivated by reducing user friction when the widespread assumption is for `react-native/metro-config` to export a complete Metro config, as with Expo/rnx-kit, and like previously understood uses of `metro-config`. See facebook/metro#1010 (comment) for further notes.

Fixes:
- facebook/metro#1010
- facebook#38069
- kristerkari/react-native-svg-transformer#276

Note that we do not intend for `react-native/metro-config` to directly export `assetExts` etc — these can be accessed on the `resolver` property from the full config object.

Changelog: [General][Changed] `react-native/metro-config` now includes all base config values from `metro-config`

Differential Revision: D47055973

fbshipit-source-id: 78b59d925be72aa42b4b9d901c6f8d174f2dbae0
@robhogan
Copy link
Contributor

It is most likely more ergonomic to have @react-native/metro-config provide the complete, extended-from-defaults config object (the original setup prior to that PR, and matching Expo).

+1. That sounds like the most practical forward fix for RN 72. Stamped internally.

huntie added a commit to huntie/react-native that referenced this issue Jun 28, 2023
Summary:
Pull Request resolved: facebook#38092

Reverts facebook#36777.

This is motivated by reducing user friction when the widespread assumption is for `react-native/metro-config` to export a complete Metro config, as with Expo/rnx-kit, and like previously understood uses of `metro-config`. See facebook/metro#1010 (comment) for further notes.

Fixes:
- facebook/metro#1010
- facebook#38069
- kristerkari/react-native-svg-transformer#276

Note that we do not intend for `react-native/metro-config` to directly export `assetExts` etc — these can be accessed on the `resolver` property from the full config object.

Changelog: [General][Changed] `react-native/metro-config` now includes all base config values from `metro-config`

Reviewed By: robhogan

Differential Revision: D47055973

fbshipit-source-id: eedc4698e651645ada46a013d3945a16965bff22
facebook-github-bot pushed a commit to facebook/react-native that referenced this issue Jun 28, 2023
Summary:
Pull Request resolved: #38092

Reverts #36777.

This is motivated by reducing user friction when the widespread assumption is for `react-native/metro-config` to export a complete Metro config, as with Expo/rnx-kit, and like previously understood uses of `metro-config`. See facebook/metro#1010 (comment) for further notes.

Fixes:
- facebook/metro#1010
- #38069
- kristerkari/react-native-svg-transformer#276

Note that we do not intend for `react-native/metro-config` to directly export `assetExts` etc — these can be accessed on the `resolver` property from the full config object.

Changelog: [General][Changed] `react-native/metro-config` now includes all base config values from `metro-config`

Reviewed By: robhogan

Differential Revision: D47055973

fbshipit-source-id: 5ad23cc9700397110de5c0e81c7d76299761ef0a
kelset pushed a commit to facebook/react-native that referenced this issue Jun 28, 2023
Summary:
Pull Request resolved: #38092

Reverts #36777.

This is motivated by reducing user friction when the widespread assumption is for `react-native/metro-config` to export a complete Metro config, as with Expo/rnx-kit, and like previously understood uses of `metro-config`. See facebook/metro#1010 (comment) for further notes.

Fixes:
- facebook/metro#1010
- #38069
- kristerkari/react-native-svg-transformer#276

Note that we do not intend for `react-native/metro-config` to directly export `assetExts` etc — these can be accessed on the `resolver` property from the full config object.

Changelog: [General][Changed] `react-native/metro-config` now includes all base config values from `metro-config`

Reviewed By: robhogan

Differential Revision: D47055973

fbshipit-source-id: 5ad23cc9700397110de5c0e81c7d76299761ef0a
@huntie
Copy link
Member

huntie commented Jun 29, 2023

This is being cherry picked into the upcoming React Native 0.72.1 release. Please make sure to upgrade @react-native/metro-config (to 0.72.1) in order to access resolver.sourceExts, resolver.assetExts.

@huntie huntie closed this as completed Jun 29, 2023
@alidavodii
Copy link

alidavodii commented Jan 5, 2024

This works on RN 0.72.3

`
const {getDefaultConfig, mergeConfig} = require('@react-native/metro-config');
/**

  • Metro configuration
  • https://facebook.github.io/metro/docs/configuration
  • @type {import('metro-config').MetroConfig}
    */
    const config = async () => {
    const defaultConfig = await getDefaultConfig(__dirname);
    const {
    resolver: {sourceExts, assetExts},
    } = defaultConfig;
    return mergeConfig(defaultConfig, {
    transformer: {
    babelTransformerPath: require.resolve('react-native-svg-transformer'),
    getTransformOptions: async () => ({
    transform: {
    experimentalImportSupport: false,
    inlineRequires: true,
    },
    }),
    },
    resolver: {
    assetExts: assetExts.filter(ext => ext !== 'svg'),
    sourceExts: [...sourceExts, 'svg'],
    },
    });
    };
    module.exports = config;
    `

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

No branches or pull requests

5 participants