Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
wants to merge 1 commit into from

Conversation

vzaidman
Copy link
Contributor

@vzaidman vzaidman commented Aug 30, 2024

The issue

Traverse in babel 7 is currently polluting the cache with BabelNode that has no hub entry (see babel/babel#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 #854 (comment) 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;

…cond copy of babel traverse

Summary:
### The issue
Traverse in babel 7 is currently polluting the cache with nodes that have no `hub` entry (see babel/babel#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 #854 (comment) 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
@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Aug 30, 2024
@facebook-github-bot
Copy link
Contributor

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

@facebook-github-bot
Copy link
Contributor

This pull request has been merged in 71c6bb5.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. fb-exported Merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants