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

ESM project support broken in 0.76/RN 0.72 #1026

Closed
oblador opened this issue Jul 7, 2023 · 7 comments
Closed

ESM project support broken in 0.76/RN 0.72 #1026

oblador opened this issue Jul 7, 2023 · 7 comments

Comments

@oblador
Copy link

oblador commented Jul 7, 2023

I'm working in an "isomorphic" project with both react-dom and react-native, where the web part is using vite that requires ESM. To make that setup work with RN/Metro that is still heavily reliant on CJS I simply renamed the metro.config.js to metro.config.cjs to force it to resolve using CJS.

With the latest version of RN (0.72.1) this setup yields No metro config found, and if I rename it to .js and use esm it throws errors originating from metro for using requires in an esm context.

My guess this is originates in an update of cosmiconfig or similar. UPDATE: It seems cosmiconfig hasn't been updated in 5 years, so shouldn't be the cause.

Even though RN setup says Node 16+, metro requires 18+ so maybe this upgrade is somehow related, but with 0.71.x it works fine.

Another solution would be to properly support ESM/TS in the config as suggested in #916

huntie added a commit to huntie/metro that referenced this issue Jul 7, 2023
Summary:
Resolves facebook#1026, resolves facebook#916.

Changelog: **[Feature]** Widen config search paths to include `metro.config.cjs`

Differential Revision: D47290079

fbshipit-source-id: d50c61f08580a04ad2de57092a0c77c5c5b0f22a
@robhogan
Copy link
Contributor

robhogan commented Jul 7, 2023

Hi @oblador - thanks for this, @huntie is already on the case adding support for metro.config.cjs, I'd like to dig in a bit as to whether something's broken.

but with 0.71.x it works fine.

This is curious. I'm not aware that metro.config.cjs has ever been supported, but if something's regressed we'll need to pick a fix back for RN72. I wonder if there might've been a build step in your project outputting a side-by-side .js?

And just to confirm so I can reproduce, when you say "use ESM", that's by setting "type": "module" in your project's package.json, or more than that?

Even though RN setup says Node 16+, metro requires 18+

Node 16 should be fine for Metro 0.76.x / RN 0.72.x. We only bumped the requirement to 18 in Metro 0.77, for RN 0.73.

@oblador
Copy link
Author

oblador commented Jul 7, 2023

I wonder if there might've been a build step in your project outputting a side-by-side .js?

Nope there isn't, just single-step babel/metro.

And just to confirm so I can reproduce, when you say "use ESM", that's by setting "type": "module" in your project's package.json, or more than that?

Exactly!

Node 16 should be fine for Metro 0.76.x / RN 0.72.x. We only bumped the requirement to 18 in Metro 0.77, for RN 0.73.

Looked into this and tracked it down to be caused by the @types/metro-config package that has a metro-config "*" dependency set up, which resolves to latest (0.77) (using yarn 1.22.17). This package isn't really needed tbf and IMHO the types should be shipped with the package itself and not separate, but I try to stick to the official template setup unless I have good reasons not to, makes upgrading a lot easier.

@oblador
Copy link
Author

oblador commented Jul 7, 2023

I had a chat with Aleks Andelkovic two weeks ago and he mentioned that some of the RN CLI parts were moving into metro, so maybe that's the cause? That the CLI used to support .cjs, but that now metro handles it, which never did? Found this file that seems to be somehow involved too, so maybe we should move the issue to the CLI repo? https://github.com/react-native-community/cli/blob/93b6977df378ef6d4504a207bf523af7bbd6ee6a/packages/cli-plugin-metro/src/tools/loadMetroConfig.ts

@oblador
Copy link
Author

oblador commented Jul 7, 2023

react-native-community/cli#1889 from @huntie seems to be the diff that introduces the error being thrown, not sure if it's the root cause.

@oblador
Copy link
Author

oblador commented Jul 7, 2023

Ah, I think I might know what's happening. Since my config was entirely vanilla and the config itself was optional, the cjs config itself was never in use, but the project compiled fine since it fell back to defaults. Now due to some change in metro, it will not work w/o a config file and therefore having a .cjs (AKA no file) will simply not work. I can't really migrate off the ESM setup due to vite/web, so I'm effectively blocked from updating.

Adding support for a .cjs config file seems like the easiest and least invasive way to unblock this (just like the related PR does).

@oliviercperrier
Copy link

oliviercperrier commented Jul 7, 2023

Got this error while upgrading my expo application to thev49 which uses RN72:

Unable to resolve "stylis" from "node_modules/@emotion/cache/dist/emotion-cache.browser.esm.js"

Not sure if this is related

@oblador
Copy link
Author

oblador commented Jul 7, 2023

@oliviercperrier It looks like it's unrelated to this issue, might be worth opening up a new issue for that with a minimal reproduction 👍

huntie added a commit to huntie/metro that referenced this issue Aug 7, 2023
Summary:
Pull Request resolved: facebook#1027

Resolves facebook#1026.

Changelog: **[Feature]** Widen config search paths to include `metro.config.cjs`

Reviewed By: robhogan

Differential Revision: D47290079

fbshipit-source-id: 3e46c954a1e1451558b14ed4558fede5f00200dd
huntie added a commit that referenced this issue Aug 7, 2023
Summary:
Pull Request resolved: #1027

Resolves #1026.

Changelog: **[Feature]** Widen config search paths to include `metro.config.cjs`

Reviewed By: robhogan

Differential Revision: D47290079

fbshipit-source-id: 3e46c954a1e1451558b14ed4558fede5f00200dd
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

Successfully merging a pull request may close this issue.

3 participants