feat: skip extraneously duplicating AST during transformation

[EvanBacon]

Summary

We split the parse/transform steps up to accommodate Hermes parsing, but this
defaults to cloning the AST which increases the transformation time by a fair amount.

You get this behavior by default when using Babel's transform method directly, which is what other bundlers do.

Test plan

Bundling three.js in production mode:

$ metro build ./example/index.js --out dist/index.js --platform ios --reset-cache --max-workers 0

Before:

babel.transform: 1.961s

After:

babel.transform: 1.047s

[EvanBacon]

Digging into the failures. Looks like the generateFunctionMap function is mutating the ast and leaving it in a malformed state.

Looks like simply running ast through traverse from @babel/traverse will cause the failure. Related babel/babel#8508.

Upgrading fixes the issue:

- "@babel/traverse": "^7.14.0",
+ "@babel/traverse": "^7.14.9",

[facebook-github-bot]

@motiz88 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

[robhogan]

Quick update - we hoped to get this in for last week's release but that wasn't possible. Updating Babel internally is causing a bit of a cascade headache.

• @babel/traverse@7.14.7 requires @babel/parser@^7.14.7 and @babel/types@^17.14.5
• @babel/parser@7.14.7 breaks on Flow-readonly private class field declarations (class Foo { +#foo; }), fixed some time before 7.20
• @babel/types@7.17.12 introduces a require('..') that some of our tooling can't resolve.
• Some combination of updates result in a bunch of (benign, I think) test snapshot changes.
• A @babel/types update breaks our undefined reference detection on Flow enums.
• ...

There's probably a combination of versions that works with a bit of lockfile hacking, but I'd rather fix the issues and make a clean upgrade to latest (7.20). Working on it..

[EvanBacon]

@robhogan any update on this?

[robhogan]

We've just come out of a holiday infra code freeze, landing those Babel updates now.

[robhogan]

The Babel ^7.20 updates have landed, but this PR is now showing failures on public CI - I'm AFK but looks like the same Babel bug that necessitated the patch update is back in the latest version.

Will try to have a look next week but just FYI in case you get chance in the meantime.

Related (?):
babel/babel#6437

[robhogan]

Follow-up:

Looks like the problem is not that the AST is mutated, but that traverse.cache holds cached state keyed on AST Node, and that state is dependent on context in which it's set - when we run through traverse for generateFunctionMappings, the cache created at traverse.cache.paths.get(sourceAst) has no hub property and is effectively polluted for Babel's purposes.

I can't find where this was ever fixed - I suspect it might not have been, but "updating" resulted in multiple @babel/traverse versions in node_modules, and the competing consumers ended up not sharing module state.

This looks like exactly the problem described by @loganfsmyth in the (still open) issue above.

A few options come to mind:

1. Work out how to call traverse within generateFunctionMappings in a way that populates the cache identically to Babel (ie, providing a hub). Babel does that here.
2. Push/pop the whole traverse.cache around generateFunctionMappings - same effect as multiple @babel/traverse installations, but a bit hacky.
3. Use traverse.clearNode(sourceAST) after traversing with generateFunctionMappings - this is a simple public API, but it costs an extra traversal.
4. Use traverse.cache.paths.delete(sourceAST) to snipe the problematic bit of the cache without clearing the whole tree - this uses a private API and leaves other paths polluted with unknown consequences.

[motiz88]

Ah, Babel path caching hell, my old nemesis. Thanks for taking a look @robhogan!

This is also a prime example of why replacing the transformer with something like SWC would be helpful from a correctness standpoint as well as (likely) performance.

[facebook-github-bot]

@robhogan has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

[robhogan]

I've gone with option 3 - the clearNode approach uses a documented API and despite the traversal it performs over the cache it's plenty quick enough (<0.1ms for three.js) to maintain the overall perf win.

If there are no objections (@motiz88, @EvanBacon) I'll merge this in time for tomorrow's scheduled Metro release.

Edit: This broke react-refresh-babel, because clearNode by default removes comments set on nodes, which is apparently how Fast Refresh works - updated to use the preserveComments option.

[facebook-github-bot]

@robhogan has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

[robhogan]

For visibility, I think #906 (option 2, effectively) is now the least-worst prerequisite.

(Clearing the node was no good either, because it wipes metadata that may be needed upstream - in particular it broke a bunch of our Jest tests because we use this transformer in Jest setup, and Jest relies on path properties.)

[facebook-github-bot]

@robhogan merged this pull request in 553241c.