facebook / metro

🚇 The JavaScript bundler for React Native
https://metrobundler.dev
MIT License
5.24k stars 626 forks source link

re-implement babel traverse cache pollution bug workaround using a second copy of babel traverse #1340

Closed vzaidman closed 2 months ago

vzaidman commented 2 months ago

The issue

Traverse in babel 7 is currently polluting the cache with BabelNode that has no hub entry (see https://github.com/babel/babel/issues/6437). Since the traverse cache is used by other babel operations (mostly transform) which might expect hub to be present, this breaks the code in all kind of unexpected scenarios. While this issue has been fixed in babel 8 (not released at this moment), it is still an issue in the latest version of babel 7.

The previous workaround

We've implemented a workaround for that issue in https://github.com/facebook/metro/pull/854#issuecomment-1336499395 however updating to the latest version of @babel/traverse, that workaround is not working 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 from the main copy of traverse (@babel/traverse) 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;
facebook-github-bot commented 2 months ago

This pull request was exported from Phabricator. Differential Revision: D61917782

facebook-github-bot commented 2 months ago

This pull request has been merged in facebook/metro@71c6bb5632ab491397d0f3f7624846c958336d6f.