@babel/types builder improvements #14519
Conversation
|
Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/51890/ |
JLHwung
force-pushed
the
babel-types-builder
branch
from
32b8d4a
to
e77587e
Compare
| @@ -28,6 +28,10 @@ function sortFieldNames(fields, type) { | |||
| }); | |||
| } | |||
|
|
|||
| function defaultExpression(field) { | |||
| return Array.isArray(field.default) ? "[]" : JSON.stringify(field.default); | |||
There was a problem hiding this comment.
Why do we need to special-case arrays?
There was a problem hiding this comment.
That was the behaviour of builder.js:
arg = Array.isArray(field.default) ? [] : field.default;I will check if we have weird field.default.
There was a problem hiding this comment.
I think we had that check to avoid sharing the same [] reference every time, but since it's now a new [] value for every call it's always a different one.
| const node: t.ArrayExpression = { | ||
| type: "ArrayExpression", | ||
| elements, | ||
| }; | ||
| validateNode(node); | ||
| return node; |
There was a problem hiding this comment.
To generate slightly smaller code, we could modify validateNode like this:
function validateNode<N extends t.Node>(node: N): N {
// todo: because keys not in BUILDER_KEYS are not validated - this actually allows invalid nodes in some cases
const keys = BUILDER_KEYS[node.type];
for (const key of keys) {
validate(node, key, node[key]);
}
return node;
}and then do
export function arrayExpression(
elements: Array<null | t.Expression | t.SpreadElement> = [],
) {
return validateNode<t.ArrayExpression>({
type: "ArrayExpression",
elements,
});
}
nicolo-ribaudo
added
the
PR: Internal 🏠
They are generated by `@babel/parser` but not returned from types builder
…s/internalSlots to [] null has been excluded by validate: arrayOfType
JLHwung
force-pushed
the
babel-types-builder
branch
from
4b252b5
to
2600b5a
Compare
| }function ${formatedBuilderNameLocal}(${defArgs.join( | ||
| ", " | ||
| )}): t.${type} { return builder.apply("${type}", arguments); }\n`; | ||
| }function ${formatedBuilderNameLocal}(${defArgs.join(", ")}): t.${type} {`; |
There was a problem hiding this comment.
The return type annotation is preserved because we skip validateNode for nodes with empty builder keys.
| }function ${formatedBuilderNameLocal}(${defArgs.join(", ")}): t.${type} {`; | ||
|
|
||
| const nodeObjectExpression = `{\n${objectFields | ||
| .map(([k, v]) => (k === v ? ` ${k},` : ` ${k}: ${v},`)) |
There was a problem hiding this comment.
Nit: since these just come from local param names, can't we make it always match?
There was a problem hiding this comment.
Most of the argument name match the AST field name. The exceptions are non-valid binding identifier names, such as CallExpression's arguments and ClassMethod's static.
github-actions
bot
added
the
outdated
This PR is reviving #13369. Further improvements includes:
The test is currently failing. However when I run the failing test, the result diverge from the whole test, implying that it may due to 1) test correlations or 2) or engine optimizer bug. Currently
Update: the last test is passing because
"2021 12 runtime errors to es2015 > invalid class decorator return"does not match any test title (according tojest-runner, which reports "1 skipped, 0 of 1 total"), butjest-light-runnerreports "1 passed".The test is failing because
babel/packages/babel-helper-create-class-features-plugin/src/fields.ts
Line 883 in 7129096
state.innerBinding, which is theidof a ClassExpression. Surprisingly the check passes for nullstate.innerBindingbecause(undefined as Binding)?.identifier !== null. This is fixed in 055933c.