-
-
Notifications
You must be signed in to change notification settings - Fork 5.6k
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.
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
Simplify (transpiled) babel-types builder wrappers #13843
Changes from 5 commits
09f4b07
10796ad
3c5847e
0c12b5d
f6acaa2
3d67e04
546fe9a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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]; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Before merging, can we discuss whether mutating There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. On the other hand, I don't see why someone would want to change There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
In our codebase we don't mutate On |
||
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}`, | ||
|
@@ -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]); | ||
} | ||
|
||
|
There was a problem hiding this comment.
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.There was a problem hiding this comment.
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 actualArray
(whether by slicing or shovelling), so the answer is: I should remove...args
from the signature as well.There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.