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

Keep upstream additionalExts when adding env files #24809

Closed
wants to merge 1 commit into from

Conversation

LinusU
Copy link
Contributor

@LinusU LinusU commented Oct 10, 2023

Why

In React Native 0.72 additionalExts is set to ['.cjs', '.mjs'] for out of the box support for these extensions, see facebook/metro@c1c6d9c. Because of this being overridden here .mjs support doesn't seem to be working out-of-the-box.

More info:

How

This change makes sure to keep the additionalExts that are set in React Native, in addition to the env-files that are added by Expo.

Test Plan

Not sure how to best test this. As described in #23322 overriding additionalExts in my own metro config fixes the problem. I'm very open to suggestions here!

Checklist

@expo-bot expo-bot added the bot: suggestions ExpoBot has some suggestions label Oct 10, 2023
@expo-bot expo-bot added bot: passed checks ExpoBot has nothing to complain about and removed bot: suggestions ExpoBot has some suggestions labels Oct 10, 2023
@@ -146,7 +143,7 @@ export function getDefaultConfig(
},
watcher: {
// strip starting dot from env files
additionalExts: envFiles.map((file: string) => file.replace(/^\./, '')),
additionalExts: ['cjs', 'mjs', ...envFiles.map((file: string) => file.replace(/^\./, ''))],
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would land in SDK 50, where mjs support is already enabled in a more robust way. Backporting would potentially break existing projects due to facebook/react-native#38628

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The change also doesn't account for Node.js file handling which requires some reordering. Unclear to me which version of React Native the Metro change was meant to support.

Copy link
Contributor

@EvanBacon EvanBacon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably should just patch the change until Expo SDK 50 lands, or use the alpha release.

@LinusU
Copy link
Contributor Author

LinusU commented Jan 23, 2024

Closing since SDK 50 is out now, and shouldn't need this 🚀

@LinusU LinusU closed this Jan 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bot: passed checks ExpoBot has nothing to complain about
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants