Simplify (transpiled) babel-types builder wrappers

[lightmare]

Q	A
Fixed Issues?	no
Patch: Bug Fix?
Major: Breaking Change?	afaik, no
Minor: New Feature?
Tests Added + Pass?	none added
Documentation PR Link
Any Dependency Changes?
License	MIT

It's a bit hacky, but bear with me. The builder function is super internal, only used from auto-generated code, so I think it can afford having a weird calling convention, such as receiving a string in this.

The end result of this PR is that transpiled babel-standalone ends up containing this:

        function arrayExpression$2(elements) {
          return builder.apply("ArrayExpression", arguments);
        }

instead of this:

        function arrayExpression$2(elements) {
          return builder.apply(void 0, ["ArrayExpression"].concat(Array.prototype.slice.call(arguments)));
        }

[codesandbox-ci]

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 546fe9a:

Sandbox	Source
babel-repl-custom-plugin	Configuration
babel-plugin-multi-config	Configuration

[babel-bot]

Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/49235/

[JLHwung]

Related: #13369
I think we can revive #13369 and get rid of arguments spread here. The non-spread version is 40% faster.

[jridgewell]

Why not just pass the (already known, because the call signature is correct) parameters into the builder?

// before
function arrayExpression$2(elements) {
  return builder.apply("ArrayExpression", arguments);
}

// after
function arrayExpression$2(elements) {
  return builder("ArrayExpression", elements);
}

[lightmare]

I think we can revive #13369 and get rid of arguments spread here. The non-spread version is 40% faster.

I've just tried transplanting the benchmark from that PR onto this one, and got this:

$ yarn node --predictable babel-types/builders/stringLiteral.mjs 
baseline stringLiteral builder: 5_672_661 ops/sec ±0.65% (0ms)
current stringLiteral builder: 8_521_407 ops/sec ±0.52% (0ms)

Why not just pass the (already known, because the call signature is correct) parameters into the builder?

I don't know the history or importance of that check, but one notable difference is that the current builder function throws if it gets too many arguments. If you only forwarded the ones named in the wrapper, that call-site error would become silent.

[nicolo-ribaudo]

I don't know the history or importance of that check

That check is important because builders don't accept params for every possible property, so it's easy to accidentally pass extra params.

[nicolo-ribaudo]

Q: what is this change needed for?

[lightmare]

Not needed, it's insignificant. I just randomly noticed the extra concat because this line was right above the builder function when I was reviewing it in the resulting babel.js.

[jridgewell]

That check is important because builders don't accept params for every possible property, so it's easy to accidentally pass extra params.

We could easily pass arguments.length to the builder implementation, if we only want one error check location. Or, we could move the check into the arrayExpression wrapper itself.

Really, I'm not sure why we need a builder function at all. Why not let arrayExpression generate it's own object? It would probably be faster to do return { type: 'ArrayExpression', elements } instead of return builder("ArrayExpression", elements }, since we wouldn't need to dynamically build the object from the BUILDER_KEYS.

[JLHwung]

Why not let arrayExpression generate it's own object

🙌 for plain object AST nodes, and we can replace startNode in babel parser, too.

we wouldn't need to dynamically build the object from the BUILDER_KEYS

It's a shotgun that t.BUILDER_KEYS can be changed by users. Just like babel/eslint-parser changes VISITOR_KEYS on the fly.

[fedeci]

Should it evaluate the args passed to the function? Otherwise if we want to keep arguments we can remove ...args: Array<any> from the signature.

Suggested change

	return builder.apply("${type}", arguments);
	return builder.apply("${type}", args);

[lightmare]

Good catch. The point of this PR is to avoid the unnecessary conversion from arguments to an actual Array (whether by slicing or shovelling), so the answer is: I should remove ...args from the signature as well.

[lightmare]

I wasn't sure whether I could change the deprecated wrapper function signatures (as these are public). If yes, then perhaps they could be as verbose as the non-deprecated ones, i.e.

`
function ${type}(${generateBuilderArgs(newType).join(", ")}): t.${type} {
`

[nicolo-ribaudo]

Yeah it is better: even if they are deprecated they still work so we can provide a good type signature.

[jridgewell]

Before merging, can we discuss whether mutating BUILD_KEYS is actually an intentional API? For the Babel 8 breaking changes, we could stop doing this and perform much faster node building by specializing each builder function. Eg, t.identifer(name) can be (a little more than) function identifier(name) { return { type: 'Identifier', name } }.

Re: #13843 (comment), #13843 (comment)

[nicolo-ribaudo]

We support swapping the parser and the generator, but not the other packages.
You can handle custom AST modifications by changing VISITOR_KEYS.

On the other hand, I don't see why someone would want to change BUILDER_KEYS (you can just directly create the node without using the builder function).

[JLHwung]

On the other hand, I don't see why someone would want to change BUILDER_KEYS (you can just directly create the node without using the builder function).

In our codebase we don't mutate BUILDER_KEYS. I think we can expand builder keys in babel-types from Babel 8 and leave a note that the BUILDER_KEYS export is for reference only, it does not change the builder behavior.

On VISITOR_KEYS, I think we can offer an overrideVisit(originalVisit) API for those mutating VISITOR_KEYS: So we can expand VISITOR_KEYS in babel-traverse, too. It should remove the slow object access due to dynamic property access on different AST nodes. TS expands visitor keys, too.