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

Add support for .mjs and .cjs file extensions #770

Closed
wants to merge 1 commit into from

Conversation

LinusU
Copy link
Contributor

@LinusU LinusU commented Feb 9, 2022

Summary

Node.js have added support for the .mjs and .cjs extensions which many packages have started using. This pull request adds these two file extensions so that packages using them can still be loaded with Metro.

This was specifically prompted by @apollo/client, a popular package that can be used from React Native, changing the file extension to .cjs which broke compatibility with Metro.

  • [Relevant if you use Apollo Client with React Native] Since Apollo Client v3.5.0, CommonJS bundles provided by @apollo/client use a .cjs file extension rather than .cjs.js, so Node.js won't interpret them as ECMAScript modules. While this change should be an implementation detail, it may cause problems for the Metro bundler used by React Native, whose resolver.sourceExts configuration does not include the cjs extension by default.

    As a workaround until this issue is resolved, you can configure Metro to understand the .cjs file extension by creating a metro.config.js file in the root of your React Native project:

    const { getDefaultConfig } = require("metro-config");
    const { resolver: defaultResolver } = getDefaultConfig.getDefaultValues();
    exports.resolver = {
      ...defaultResolver,
      sourceExts: [
        ...defaultResolver.sourceExts,
        "cjs",
      ],
    };

Fixes #535
Fixes #59
Fixes #60

Test plan

In a temporary directory:

  1. npx react-native@latest init TestSourceExts
  2. cd TestSourceExts
  3. npx react-native run-ios
    1. Check that project works
    2. Close everything opened
  4. yarn add @apollo/client graphql
  5. Add import { ApolloProvider } from '@apollo/client' to top of app.js
  6. Add the following snippet to the <App />
    <Section title="Apollo Import Test">
      Should be function: {typeof ApolloProvider}
    </Section>
  7. npx react-native run-ios
    1. Observe the error
    2. Close everything opened
  8. Manually patch node_modules/metro-config/src/defaults/defaults.js
    - exports.sourceExts = ["js", "json", "ts", "tsx"];
    + exports.sourceExts = ["js", "json", "ts", "tsx", "mjs", "cjs"];
  9. npx react-native run-ios
    1. Observe everything gloriously working
    2. Celebrate 🎉

@facebook-github-bot facebook-github-bot added CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team. labels Feb 9, 2022
@facebook-github-bot
Copy link
Contributor

@GijsWeterings has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@LinusU
Copy link
Contributor Author

LinusU commented Feb 18, 2022

@GijsWeterings thank you for importing this! It seems like some internal build is failing, is there anything that I could help with?

@robhogan
Copy link
Contributor

Hi @LinusU - the test failure was unrelated - apologies for the delay. We're discussing a slightly different approach here because currently adding to sourceExts does two things:

  1. Includes additional extensions in the in-memory file map / watcher, so that Metro is aware of their existence
  2. Tries the extensions when resolving imports / requires, eg we'll look for foo.js when you require('./foo')

Only the first is necessary in this case, because .cjs/.mjs extensions must be given explicitly when using require/import - in other words require('./foo') should never resolve to ./foo.cjs. Obviously we'd like Metro's defaults to stay as close to node's behaviour as reasonably possible, and there's a performance consideration in having unnecessary resolution candidates too.

We're going to tweak the config format here to allow options for each extension (so we can have 1. without 2.) - something like sourceExts: Array<string | [string, { resolveImplicit: boolean }]>. Do you mind if I push this change to your PR?

@LinusU
Copy link
Contributor Author

LinusU commented Feb 18, 2022

Obviously we'd like Metro's defaults to stay as close to node's behaviour as reasonably possible

This sounds great 👍

We're going to tweak the config format here to allow options for each extension (so we can have 1. without 2.) - something like sourceExts: Array<string | [string, { resolveImplicit: boolean }]>. Do you mind if I push this change to your PR?

Not at all, that would be super! 👏

@mattwr18
Copy link

mattwr18 commented Mar 9, 2022

I'm not sure if this is the best place to ask this question, but in the hopes it might save someone else considerable time debugging, I'll ask.

We recently added @apollo/client to our react native project and ran into the .cjs issue. We used the referenced workaround that is in the Apollo CHANGELOG and ran into an issue with another package, pubnub.

Somehow, it was detecting that we were in a web environment 🤷
In the end, we removed the line spreading the ...defaultResolver and just spread the sourceExts and added 'cjs'.

Any idea what in the defaultResolver could have caused this? I noticed there were platforms [ 'ios', 'android', 'windows', 'web' ], but removing windows and web didn't seem to solve the issue. Maybe, the resolverMainFields: [ 'browser', 'main' ] would make some undesirable impact?

Anyways, sorry if this isn't the place for this discussion.

@tido64
Copy link
Contributor

tido64 commented May 6, 2022

@robhogan, @motiz88: Any chance we can get this in? We are hitting this internally as well. I'm aware that we can configure sourceExts, but it would be nice if we supported this out of box.

cc @asmundg

@SohelIslamImran
Copy link

If you are using expo, to resolve this issue, create a metro.config.js file in the project root. In the file add the file extension cjs. details

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

const defaultConfig = getDefaultConfig(__dirname);

defaultConfig.resolver.assetExts.push("cjs");

module.exports = defaultConfig;

image

@huntie
Copy link
Member

huntie commented Jul 28, 2022

We have now landed out-of-the-box support for .cjs and .mjs in c1c6d9c via the watcher.additionalExts option. Thanks @LinusU for opening this PR, which has been a great reference for others and brought this to our attention internally 🙂

@huntie huntie closed this Jul 28, 2022
@LinusU
Copy link
Contributor Author

LinusU commented Aug 1, 2022

@huntie that's awesome! I'm glad to hear it 🎉

@LinusU LinusU deleted the patch-1 branch August 1, 2022 16:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add .cjs and .mjs files support Is the .mjs file extension resolved by metro bundler?
7 participants