feat(metro-config): add custom metro transform worker #25833
Merged
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
fix tests Update metro-transform-worker.test.ts Update metro-transform-worker.test.ts.snap wip wip
EvanBacon
requested review from
brentvatne,
ide,
byCedric and
marklawlor
as code owners
Co-authored-by: Expo Bot <34669131+expo-bot@users.noreply.github.com>
|
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) 👇
|
github-actions
bot
added
bot: fingerprint changed
and removed
bot: fingerprint compatible
labels
marklawlor
approved these changes
Dec 11, 2023
EvanBacon
changed the title
This was referenced Dec 12, 2023
kitten
added a commit
that referenced
this pull request
Dec 14, 2023
…-worker with mappings tests (#25872) # Why - Closes ENG-10873 - This reverts the lock on `@babel/generator@~7.20.5` to `@babel/generator@^7.20.5` to allow upgrades past `7.21.0` - This was previously put in place in #25833 and this is a follow-up PR - `@babel/generator@7.21.0` shipped a fix that added names of “friendly call frames” to its source maps (babel/babel#15022). This caused the source maps to change to include internal names for in-scope identifiers such as `globalThis` and `_interopRequireDefault` - This was a completely backwards-compatible change for us, but this was obscured by source map output being a little hard to read When applied, this PR replaces our source map snapshots with tests that check exact identifiers in the input and output against their mappings and names. This should make it easier to identify changes to the source maps and allow us to debug them more easily in the future. A note has been added to any assertions in the tests that break when downgrading below `7.21.0`, but for now, I haven't added any additional tests that account for this, since I'm not expecting us to downgrade to the lower version again, and to instead keep all `@babel/*` packages in `@expo/metro-config` upgraded in lockstep. # How - All source-map snapshots for `metro-transformer-worker` have been deleted - `@jridgewell/trace-mapping` has been added to parse the worker’s source maps (with a small converter function) - This is an alternative to the ubiquitous `source-map` package, and it’s also used in Babel. It avoids parsing source maps in wasm. - When adding `source-map` instead, we’d have to work around our virtual FS mock - All source-map snapshots have been replaced with checks for both input and output identifiers against the actual source-maps - Additional tests have been added to check for internal globals, which previously caused the source maps to break # Test Plan **Note:** When testing this, placing the pinned version back to `@babel/generator@~7.20.5` causes some `name` properties to be missing in the source-map output for injected globals, as expected. # Checklist - [ ] Documentation is up to date to reflect these changes (eg: https://docs.expo.dev and README.md). - [ ] Conforms with the [Documentation Writing Style Guide](https://github.com/expo/expo/blob/main/guides/Expo%20Documentation%20Writing%20Style%20Guide.md) - [ ] This diff will work correctly for `npx expo prebuild` & EAS Build (eg: updated a module plugin).
onizam95
pushed a commit
to onizam95/expo-av-drm
that referenced
this pull request
Jan 15, 2024
# Why - Alternative to expo#25774 -- we'll fork the transform worker and serializer (already done) and implement custom asset hashing that is platform-specific and won't modify native output. - I've been meaning to do this for a while since we already moved the inline constants plugin to babel-preset-expo to fix Platform shaking. Because of this, ~~we can remove an entire babel transform pass in development when inline requires and experimental export support are disabled (default, possibly forever).~~ need to keep this enabled for some mystery reason, will re-approach in the future. - To emulate the default behavior prescribed by the upstream tests, I've added support for automatically disabling the import/export transform based on if `experimentalImportSupport` is enabled. We do this by passing the Babel-standard caller flag `supportsStaticESM` to `babel-preset-expo`, which actually makes the entire cjs conversion bundler agnostic (for bundlers that support Babel). - I've locked the `babel/generator` version as there appears to be a bug (or fix?) in the newer patch that silently makes source maps only apply to the output code and not the input, which breaks the upstream tests. This could possibly be the missing solution to my tree-shaking branch and why I couldn't get the transforms to match in the serializer. - Most of this PR is just adding types for everything and porting the tests. - This PR adds built-in centralized hashing for production export assets on web-only. We don't need to modify the output pipeline anymore since everything is handled in the transformer logic. --------- Co-authored-by: Expo Bot <34669131+expo-bot@users.noreply.github.com>
onizam95
pushed a commit
to onizam95/expo-av-drm
that referenced
this pull request
Jan 15, 2024
…-worker with mappings tests (expo#25872) # Why - Closes ENG-10873 - This reverts the lock on `@babel/generator@~7.20.5` to `@babel/generator@^7.20.5` to allow upgrades past `7.21.0` - This was previously put in place in expo#25833 and this is a follow-up PR - `@babel/generator@7.21.0` shipped a fix that added names of “friendly call frames” to its source maps (babel/babel#15022). This caused the source maps to change to include internal names for in-scope identifiers such as `globalThis` and `_interopRequireDefault` - This was a completely backwards-compatible change for us, but this was obscured by source map output being a little hard to read When applied, this PR replaces our source map snapshots with tests that check exact identifiers in the input and output against their mappings and names. This should make it easier to identify changes to the source maps and allow us to debug them more easily in the future. A note has been added to any assertions in the tests that break when downgrading below `7.21.0`, but for now, I haven't added any additional tests that account for this, since I'm not expecting us to downgrade to the lower version again, and to instead keep all `@babel/*` packages in `@expo/metro-config` upgraded in lockstep. # How - All source-map snapshots for `metro-transformer-worker` have been deleted - `@jridgewell/trace-mapping` has been added to parse the worker’s source maps (with a small converter function) - This is an alternative to the ubiquitous `source-map` package, and it’s also used in Babel. It avoids parsing source maps in wasm. - When adding `source-map` instead, we’d have to work around our virtual FS mock - All source-map snapshots have been replaced with checks for both input and output identifiers against the actual source-maps - Additional tests have been added to check for internal globals, which previously caused the source maps to break # Test Plan **Note:** When testing this, placing the pinned version back to `@babel/generator@~7.20.5` causes some `name` properties to be missing in the source-map output for injected globals, as expected. # Checklist - [ ] Documentation is up to date to reflect these changes (eg: https://docs.expo.dev and README.md). - [ ] Conforms with the [Documentation Writing Style Guide](https://github.com/expo/expo/blob/main/guides/Expo%20Documentation%20Writing%20Style%20Guide.md) - [ ] This diff will work correctly for `npx expo prebuild` & EAS Build (eg: updated a module plugin).
EvanBacon
added a commit
that referenced
this pull request
Feb 12, 2024
Closes ENG-10870 # Why We want to remove an extraneous Babel pass in the `metro-transform-worker`, as a follow-up task to #25833. The relevant todo is in the worker itself: https://github.com/expo/expo/blob/b80decc29ff61832ee535d41741f7ce584f29a76/packages/%40expo/metro-config/src/transform-worker/metro-transform-worker.ts#L222-L223 Previously, we couldn't remove this pass as without it `dev: false` builds would be missing `_interopRequireWildcard` and `_interopRequireDefault` Babel helper inline functions. However, removing it gets rid of an unnecessary `transformFromAstSync` call which may run without plugins and clones the entire Babel Input AST. # How Investigating the issue, it turns out that Babel has a cache for AST nodes that keeps track of previously created `path` and `scope` objects. This is kept on `@babel/traverse`'s `traverse.cache`. The `cloneInputAst: true` option was needed on at least one `transformFromAstSync` because it busts this path/scope cache. It internally performs a simple deep-clone of the AST and means that the next traverse call doesn't have a path/scope cache to fall back on. This will cause Babel to recrawl the scope of an AST. This basically means that this problem has two sides to it, which work together to cause the bug that removing `cloneInputAst: true` triggers: 1. A Babel plugin in our `babel-transformer` is corrupting the scope early on (as part of `@react-native/babel-preset`) 2. A Babel plugin in one of our `transformFromAstSync` passes is corrupting the AST due to the corrupted scope. In our case, the Babel plugin corrupting the scope is `@babel/plugin-transform-modules-commonjs`. This plugin uses `hub.addHelper("interopRequireWildcard")` and similar calls that inject Babel helpers. In most cases, these calls to `hub.addHelper` don't update any `referencePaths` and thus the function declarations it injects are un-referenced from the perspective of Babel’s scope. The Babel plugin corrupting the AST and removing the above function declarations is Metro’s `constantFoldingPlugin`, which is only used when `dev: true` is set. This plugin, among other things, removes unused declarations. When it sees the inserted function declarations with the _corrupted scope_, it removes them. There are several ways to ensure that the scope is correct after the initial Babel plugins are run, either: - each Babel plugin that uses `hub.addHelper` (or inserts declaration without letting traversal continue) must update the scope manually. - This bug may go away in Babel 8 and we don't control all plugins that a user may use, so this isn't a suitable fix - the AST may be cloned using `cloneInputAst: true` on the `constantFoldingPlugin` instead, or the via a `deepClone` utility. - Basically, we could also move `cloneInputAst: true` to the `dev: true` pass. - `traverse.cache.clear()` may be called to clear the entire traversal cache - This could have performance implications that are hard to analyse. - `programPath.scope.crawl()` can be called in or before the `constantFoldingPlugin` to make sure it always has an up-to-date scope. This PR, when applied, uses the last solution in the above list. It also disables `cloneInputAst: true` on all transform passes, since only the `constantFoldingPlugin` uses the scope to alter the AST in a way that could break the output code. # Test Plan - A new unit test has been added for `metro-transform-worker` that uses the `constantFoldingPlugin` and checks the output for code that will generate a `_interopRequireWildcard` declaration - This ensures we don't break the `constantFoldingPlugin` again without having to run E2E tests - I also used this unit test to test that reverting this fix breaks, as expected - I manually tracked down the bug by disabling `cloneInputAst` everywhere and then selectively stepping through code. Removing `constantFoldingPlugin` also fixes the output. - By tracking down some related Metro issues, I tested `traverse.cache.clear()` in conjunction with other changes # Checklist - [ ] Documentation is up to date to reflect these changes (eg: https://docs.expo.dev and README.md). - [ ] Conforms with the [Documentation Writing Style Guide](https://github.com/expo/expo/blob/main/guides/Expo%20Documentation%20Writing%20Style%20Guide.md) - [ ] This diff will work correctly for `npx expo prebuild` & EAS Build (eg: updated a module plugin). --------- Co-authored-by: evanbacon <bacon@expo.io>
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
we can remove an entire babel transform pass in development when inline requires and experimental export support are disabled (default, possibly forever).need to keep this enabled for some mystery reason, will re-approach in the future.experimentalImportSupportis enabled. We do this by passing the Babel-standard caller flagsupportsStaticESMtobabel-preset-expo, which actually makes the entire cjs conversion bundler agnostic (for bundlers that support Babel).babel/generatorversion as there appears to be a bug (or fix?) in the newer patch that silently makes source maps only apply to the output code and not the input, which breaks the upstream tests. This could possibly be the missing solution to my tree-shaking branch and why I couldn't get the transforms to match in the serializer.