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.

Sign up for GitHub

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

Jump to bottom

Simplify (transpiled) babel-types builder wrappers #13843

Merged
merged 7 commits into from Oct 21, 2021
Merged

Simplify (transpiled) babel-types builder wrappers #13843

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
09f4b07
babel-types: simplify generated builder wrappers ...
lightmare Jun 4, 2021
10796ad
replace `.forEach` with a loop to avoid closure over `arguments`
lightmare Oct 12, 2021
3c5847e
replace `for..of` loop with `for..in` when iterating over fresh object
lightmare Oct 12, 2021
0c12b5d
concat multiple arrays at once
lightmare Oct 13, 2021
f6acaa2
chore: gulp generate-type-helpers
lightmare Oct 13, 2021
3d67e04
improve deprecated builder type signatures
lightmare Oct 16, 2021
546fe9a
chore: gulp generate-type-helpers
lightmare Oct 16, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
6 changes: 3 additions & 3 deletions packages/babel-types/scripts/generators/builders.js
View file
Open in desktop
Expand Up @@ -100,7 +100,7 @@ import type * as t from "../..";
formatedBuilderNameLocal === formatedBuilderName ? "export " : ""
}function ${formatedBuilderNameLocal}(${defArgs.join(
", "
)}): t.${type} { return builder("${type}", ...arguments); }\n`;
)}): t.${type} { return builder.apply("${type}", arguments); }\n`;
if (formatedBuilderNameLocal !== formatedBuilderName) {
output += `export { ${formatedBuilderNameLocal} as ${formatedBuilderName} };\n`;
}
Expand All @@ -119,9 +119,9 @@ import type * as t from "../..";
const newType = definitions.DEPRECATED_KEYS[type];
const formatedBuilderName = formatBuilderName(type);
output += `/** @deprecated */
function ${type}(...args: Array<any>): any {
function ${type}(${generateBuilderArgs(newType).join(", ")}): t.${type} {
console.trace("The node type ${type} has been renamed to ${newType}");
return builder("${type}", ...args);
return builder.apply("${type}", arguments);
Copy link
Member

@fedeci fedeci Oct 16, 2021 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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} {
`

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}
export { ${type} as ${formatedBuilderName} };\n`;
// This is needed for backwards compatibility.
Expand Down
21 changes: 10 additions & 11 deletions packages/babel-types/src/builders/builder.ts
View file
Open in desktop
Expand Up @@ -2,12 +2,10 @@ import { NODE_FIELDS, BUILDER_KEYS } from "../definitions";
import validate from "../validators/validate";
import type * as t from "..";

export default function builder<T extends t.Node>(
type: T["type"],
...args: Array<any>
): T {
export default function builder<T extends t.Node>(this: T["type"]): T {
const type = this;
const keys = BUILDER_KEYS[type];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

const countArgs = args.length;
const countArgs = arguments.length;
if (countArgs > keys.length) {
throw new Error(
`${type}: Too many arguments passed. Received ${countArgs} but can receive no more than ${keys.length}`,
Expand All @@ -16,21 +14,22 @@ export default function builder<T extends t.Node>(

const node = { type };

let i = 0;
keys.forEach(key => {
for (let i = 0; i < keys.length; ++i) {
const key = keys[i];
const field = NODE_FIELDS[type][key];

let arg;
if (i < countArgs) arg = args[i];
if (i < countArgs) arg = arguments[i];
if (arg === undefined) {
arg = Array.isArray(field.default) ? [] : field.default;
}

node[key] = arg;
i++;
});
}

for (const key of Object.keys(node)) {
// (assume all enumerable properties are own)
// eslint-disable-next-line guard-for-in
for (const key in node) {
validate(node as t.Node, key, node[key]);
}

Expand Down