From bface4b9148f698cd9a3494dcca3e217a8d27fb7 Mon Sep 17 00:00:00 2001 From: Vitali Zaidman Date: Fri, 30 Aug 2024 03:30:26 -0700 Subject: [PATCH] re-implement babel traverse cache pollution bug workaround using a second copy of babel traverse Summary: ### The issue Traverse in babel 7 is currently polluting the cache with nodes that have no `hub` entry (see https://github.com/babel/babel/issues/6437). Since the traverse cache is used by other babel operations that expect `hub` to be present, this breaks the code in all kind of unexpected scenarios. While this issue is fixed in babel 8 (not released at this moment), it's still an issue in the latest version of babel 7. ### The previous workaround We implemented a workaround for it in https://github.com/facebook/metro/pull/854#issuecomment-1336499395 however in the latest version of `babel/traverse`, that workaround cannot be used anymore (more about that below). ### The new workaround Instead, we are implementing a different workaround that installs traverse twice, including installing it's cache file twice. We use the second copy of traverse `babel/traverse--for-generate-function-map` only in `forEachMapping`, allowing the rest of the system to use the traverse caching without the pollution issue mentioned above. ### Why the previous workaround stopped working Due to the use of a `let export` in the latest version of traverse cache, and how it's used in the latest version, we can't re-write `traverse.cache.path` anymore. This cache is exported with a `let export`: https://github.com/babel/babel/blob/5ebab544af2f1c6fc6abdaae6f4e5426975c9a16/packages/babel-traverse/src/cache.ts#L6-L20 And it compiles to: ``` let pathsCache = exports.path = new WeakMap(); function clearPath() { exports.path = pathsCache = new WeakMap(); } ``` and then used like this: ``` function getCachedPaths(hub, parent) { // ... pathsCache.get(hub); // ... } ``` Which means that re-writing the export like we used to do breaks the traverse cache because `exports.path` is re-written, but not `pathsCache` while the latter is used inside the file: ``` const previousCache = traverse.cache.path; traverse.cache.clearPath(); traverse(ast, { /* settings */ }); // this line is breaking the traverse cache traverse.cache.path = previousCache; ``` Differential Revision: D61917782 --- packages/metro-source-map/package.json | 1 + .../metro-source-map/src/generateFunctionMap.js | 13 ++++--------- yarn.lock | 2 +- 3 files changed, 6 insertions(+), 10 deletions(-) diff --git a/packages/metro-source-map/package.json b/packages/metro-source-map/package.json index 619fa4723d..fb462dcc53 100644 --- a/packages/metro-source-map/package.json +++ b/packages/metro-source-map/package.json @@ -13,6 +13,7 @@ }, "dependencies": { "@babel/traverse": "^7.20.0", + "@babel/traverse--for-generate-function-map": "npm:@babel/traverse@^7.20.0", "@babel/types": "^7.20.0", "flow-enums-runtime": "^0.0.6", "invariant": "^2.2.4", diff --git a/packages/metro-source-map/src/generateFunctionMap.js b/packages/metro-source-map/src/generateFunctionMap.js index 3e6e980bb7..d3d5444d77 100644 --- a/packages/metro-source-map/src/generateFunctionMap.js +++ b/packages/metro-source-map/src/generateFunctionMap.js @@ -17,7 +17,8 @@ import type {NodePath} from '@babel/traverse'; import type {Node} from '@babel/types'; import type {MetroBabelFileMetadata} from 'metro-babel-transformer'; -import traverse from '@babel/traverse'; +// $FlowFixMe[cannot-resolve-module] - resolves to @babel/traverse +import traverseSecondInstallation from '@babel/traverse--for-generate-function-map'; import { isAssignmentExpression, isClassBody, @@ -220,13 +221,8 @@ function forEachMapping( ) { const visitor = getFunctionMapVisitor(context, pushMapping); - // Traversing populates/pollutes the path cache (`traverse.cache.path`) with - // values missing the `hub` property needed by Babel transformation, so we - // save, clear, and restore the cache around our traversal. - // See: https://github.com/facebook/metro/pull/854#issuecomment-1336499395 - const previousCache = traverse.cache.path; - traverse.cache.clearPath(); - traverse(ast, { + // TODO: improve comment + traverseSecondInstallation(ast, { // Our visitor doesn't care about scope noScope: true, @@ -234,7 +230,6 @@ function forEachMapping( Program: visitor, Class: visitor, }); - traverse.cache.path = previousCache; } const ANONYMOUS_NAME = ''; diff --git a/yarn.lock b/yarn.lock index 4bbf47f348..852175876a 100644 --- a/yarn.lock +++ b/yarn.lock @@ -1089,7 +1089,7 @@ "@babel/parser" "^7.22.5" "@babel/types" "^7.22.5" -"@babel/traverse@^7.19.1", "@babel/traverse@^7.20.0", "@babel/traverse@^7.20.1", "@babel/traverse@^7.20.5", "@babel/traverse@^7.4.3": +"@babel/traverse--for-generate-function-map@npm:@babel/traverse@^7.20.0", "@babel/traverse@^7.19.1", "@babel/traverse@^7.20.0", "@babel/traverse@^7.20.1", "@babel/traverse@^7.20.5", "@babel/traverse@^7.4.3": version "7.20.5" resolved "https://registry.yarnpkg.com/@babel/traverse/-/traverse-7.20.5.tgz#78eb244bea8270fdda1ef9af22a5d5e5b7e57133" integrity sha512-WM5ZNN3JITQIq9tFZaw1ojLU3WgWdtkxnhM1AegMS+PvHjkM5IXjmYEGY7yukz5XS4sJyEf2VzWjI8uAavhxBQ==