Pass Babel AST directly

[michael-ciniawsky]

I'm submitting a feature request

Webpack Version:

v4.0.0 (@next)

Babel Core Version:

v7.0.0 (@next)

Babel Loader Version:

v8.0.0

Current behavior:

Pass result.code {String}

Desired behavior:

Pass result.ast as metadata cb(null, code, map, { webpackAST: ast /* result.ast */ }) {Object}

⚠️ Needs to be compatible to the Acorn AST (ESTree) (webpack #5925)

• What is the motivation / use case for changing the behavior?

Avoid unnecessary parsing, if possible

[xtuc]

I started to work on this.

The only change in babel-loader is to emit an AST instead of the source.

We need to convert our Babel AST to the ESTree, which shouldn't be too hard.

[hzoo]

Depends on how it works, may need it to be transformed both ways if Babel loader is passed an ast itself, easiest thing to do to output the ast would be if we wrote a Babel transform for it but not sure we can handle that with different nodes atm. We have the estree logic in Babylon, babel-eslint, espree

[xtuc]

I assumed that Babel would parse and apply transformations. But it seems that works only that way? I don't see where the loader has access to the ast?

[michael-ciniawsky]

It doesn't have access atm,

- function loader (src, map)
+ function loader (src, map, { webpackAST })

But if there needs to be a Tree Adapter of some sort the question is if this isn't actually slower then letting webpack parse it again :)

[xtuc]

But if there needs to be a Tree Adapter of some sort the question is if this isn't actually slower then letting webpack parse it again :)

I'm sure it will be faster. Parsing text (+ grammar) is way slower than just applying some mutations on the AST and given that the ASTs aren't so much different.

[hzoo]

https://twitter.com/loganfsmyth/status/929564354571157504

[xtuc]

It seems that a converter already exists https://www.npmjs.com/package/babel-to-estree

[hzoo]

We have the estree logic in Babylon, babel-eslint, espree

It is a fork of what we already had in babel-eslint and both might be out of date

[loganfsmyth]

In recently playing around with caching in babel-core, I've noticed that my test logic (so I'd assume babel-loader too) spends a decent chunk of time in JSON.stringify and JSON.parse on Babel's result object because of the AST. I wonder how performance gains from webpackAST would compare to the gains of passing ast: false to Babel, so it doesn't have to cache the AST as JSON, but then also wouldn't have it to pass off to Webpack. Webpack would have to parse Babel's output string, but it'd mean we don't have to re-parse the AST from JSON. While simpler to parse, the JSON is dramatically larger. Not sure where that tradeoff would be.

[DrewML]

I have a few different POCs of this locally with webpack@next and various different ways of going from Babylon >> ESTree. Just looking for some medium/large sized projects to profile on to determine if this is worthwhile to bring into the project.

[loganfsmyth]

I'll just register that if we do find we want to land this, I think I'd push for it being opt-in. When caching lands in core, it'll mean we just have to spend tons of time parsing JSON instead of tons of time parsing JS, and I'm pretty sure the AST JSON is so large that it takes longer to parse than the actual JS content would take.

I could see this being a performance boost if the user isn't using any caching, but I'm not sure that's a case we should bother optimizing for?

[DrewML]

I think I'd push for it being opt-in.

Agreed. I'm still skeptical this will have any real perf benefits, but I wanted to actually profile it so we can make a decision, document it, and put it to rest 😄. Lots of discussions about it in the past, hoping to bring those discussions to a conclusion.

[DrewML]

Looks like this is more challenging that just passing an ESTree-compatible AST back to webpack.

I created the lazy-babylon-to-estree package to do the minimum amount of changes needed for webpack to be happy.

After running tests on some larger projects, I hit a major blocker: anytime a transform creates a new node, that new node won't have a start/end/range. Webpack relies on those values existing, since it does string-replacement on sources rather than a full print from the AST.

Example <Foo />is going to transform to React.createElement(Foo, null). webpack expects that member expression to have a range because, when the React import declaration is swapped, webpack needs to replace usages of the React identifier.

If someone is interested in trying this locally, npm i lazy-babylon-to-estree, then try this patch:

diff --git a/src/index.js b/src/index.js
index 11e4f10..5ba7a6f 100644
--- a/src/index.js
+++ b/src/index.js
@@ -8,6 +8,7 @@ const read = require("./utils/read");
 const resolveRc = require("./resolve-rc.js");
 const pkg = require("../package.json");
 const fs = require("fs");
+const toESTree = require("lazy-babylon-to-estree");

 /**
  * Error thrown by Babel formatted to conform to Webpack reporting.
@@ -74,6 +75,7 @@ const transpile = function(source, options) {
   const code = result.code;
   const map = result.map;
   const metadata = result.metadata;
+  const ast = result.ast;

   if (map && (!map.sourcesContent || !map.sourcesContent.length)) {
     map.sourcesContent = [source];
@@ -85,6 +87,7 @@ const transpile = function(source, options) {
     code: code,
     map: map,
     metadata: metadata,
+    ast: ast,
   };
 };

@@ -180,9 +183,11 @@ module.exports = function(source, inputSourceMap) {
     );
   }

-  const { code, map, metadata } = transpile(source, options);
+  const { code, map, metadata, ast } = transpile(source, options);

   metadataSubscribers.forEach(s => passMetadata(s, this, metadata));

-  this.callback(null, code, map);
+  this.callback(null, code, map, {
+    webpackAST: ast && toESTree(ast),
+  });

You'll need to pass the following options to babel-loader:

options: {
	parserOpts: {
		ranges: true
	}
}

[loganfsmyth]

After running tests on some larger projects, I hit a major blocker: anytime a transform creates a new node, that new node won't have a start/end/range.

Great point, and thinking on that even more, it's not even just that. The location ranges in the AST reference the locations in the original source content passed to the loader, not the position in the resulting code string returned from the loader.

[DrewML]

The location ranges in the AST reference the locations in the original source content passed to the loader, not the position in the resulting code string returned from the loader.

Oof, I hadn't even considered that. I hate to say it, but I think this idea is pretty much dead in the water.

I doubt we'd want to take the perf hit and do the massive amount of work in core of updating the AST locations as we go just for this, when there are probably other changes that would have bigger (positive) impacts on build perf.

[jameswomack]

I'd love to have an opt-in version of this. It'd enable me to process/act on call expressions (or other fragments) that the Webpack AST hooks aren't able to reach, including:

• Those in JSX
• Those in if statements, switch statements deeper in module hierarchies etc.

[reuwi]

It's been 5 years since this issue was created...so has any progress?