Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
fix(metro-config): Remove extraneous Babel pass (#25930)
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>
- Loading branch information
