feat(metro-config)!: enable experimentalImportSupport by default
#30005
+2,118
−2,110
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
The Pull Request introduced fingerprint changes against the base commit: db6f679 Fingerprint diff[
{
"type": "dir",
"filePath": "../../packages/expo-dev-launcher",
"reasons": [
"expoAutolinkingIos",
"expoAutolinkingAndroid"
],
"hash": "bec35d3b4a8ed9bf794c217b4e7b6dea2d0019bb"
}
]Generated by PR labeler 🤖 |
Co-authored-by: Expo Bot <[email protected]>
|
Hi there! 👋 I'm a bot whose goal is to ensure your contributions meet our guidelines. I've found some issues in your pull request that should be addressed (click on them for more details) 👇
|
This was referenced Jun 25, 2024
EvanBacon
added a commit
that referenced
this pull request
Jun 27, 2024
…0010) # Why - We are currently hiding this issue but it becomes clear when we support ESM better #30005 --------- Co-authored-by: Expo Bot <[email protected]>
github-merge-queue bot
pushed a commit
to software-mansion/react-native-reanimated
that referenced
this pull request
Jun 28, 2024
<!-- Thanks for submitting a pull request! We appreciate you spending the time to work on these changes. Please follow the template so that the reviewers can easily understand what the code changes affect. --> ## Summary React Native Babel preset uses a loose plugin to convert imports to commonjs, we're investigating switching to a more standard approach in Expo CLI here expo/expo#30005 which aligns with how Metro is used at Meta. When running on the kitchen sink, I found that there was an invalid expression in reanimated where it exports null and then destructures null. This error didn't occur previously because the commonjs plugin deferred access to the null object until later. **Before** ```js var _Math = _$$_REQUIRE(_dependencyMap[0]); // Later ... _Math.add ``` This coincidentally didn't throw because the deferred code is never accessed. **After** ```js var add = _$$_REQUIRE(_dependencyMap[0]).add; ``` This throws because the `null` is accessed when the module is loaded. ## Test plan Bundle a Metro web project with experimentalImportSupport enabled: ```js config.transformer.getTransformOptions = async () => ({ transform: { experimentalImportSupport: true, inlineRequires: false, }, }); ``` Then run `npx serve dist` -> Error was there but not anymore.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Why
In SDK 52 and greater, we should try to enable
experimentalImportSupportby default. This feature converts import/export syntax to use Metro runtime equivalents in a secondary Babel pass. This aligns with how Metro is used internally at Meta, and fixes some broken behavior in the Metro transform pipeline.When
experimentalImportSupportis disabled, the babel plugins@babel/plugin-transform-modules-commonjsand@babel/plugin-proposal-export-default-fromare used to emulate the behavior but with@babel/runtimeequivalents. This leads to substantially more imports and breaks the"use strict"directive injection since all modules are viewed as commonjs.When this flag is enabled
lazyImportsis silently ignored. Which raises some other questions. It appears we currently discard the internal list of lazy imports by React Native. I'd imagine the RN list is there to emulate a subset ofinlineRequiresbehavior. When bundling a React Native iOS app however, it doesn't appear that any of the modules in the RN list are ever invoked. This seems like an artifact of when React Native migrated from using Haste-style importsViewto standard imports like./View.For us, the only notable change is that imports on
expo,expo-asset, andexpo-task-managerwill no longer default to being lazily initialized.expoandexpo-assetsshould never have been in this list since they both contain root side-effects that must be imported at an exact time. Not sure if anything breaks by movingexpo-task-manager, can't imagine how it might.Syntax changes
module(like we do in expo-maps) will not work anymore.Test Plan