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

fix: generated builder parameter should respect builder keys #11002

Merged
merged 1 commit into from Jan 13, 2020
Merged

fix: generated builder parameter should respect builder keys #11002

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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: 4 additions & 2 deletions packages/babel-types/scripts/generators/flow.js
View file
Open in desktop
Expand Up @@ -54,6 +54,7 @@ for (const type in t.NODE_FIELDS) {

const struct = ['type: "' + type + '";'];
const args = [];
const builderNames = t.BUILDER_KEYS[type];

Object.keys(t.NODE_FIELDS[type])
.sort((fieldA, fieldB) => {
Expand All @@ -80,8 +81,9 @@ for (const type in t.NODE_FIELDS) {
if (typeAnnotation) {
suffix += ": " + typeAnnotation;
}

args.push(t.toBindingIdentifierName(fieldName) + suffix);
if (builderNames.includes(fieldName)) {
args.push(t.toBindingIdentifierName(fieldName) + suffix);
}

if (t.isValidIdentifier(fieldName)) {
struct.push(fieldName + suffix + ";");
Expand Down
27 changes: 15 additions & 12 deletions packages/babel-types/scripts/generators/typescript.js
View file
Open in desktop
Expand Up @@ -56,6 +56,7 @@ const lines = [];
for (const type in t.NODE_FIELDS) {
const fields = t.NODE_FIELDS[type];
const fieldNames = sortFieldNames(Object.keys(t.NODE_FIELDS[type]), type);
const builderNames = t.BUILDER_KEYS[type];

const struct = ['type: "' + type + '";'];
const args = [];
Expand All @@ -75,18 +76,20 @@ for (const type in t.NODE_FIELDS) {
typeAnnotation += " | null";
}

if (areAllRemainingFieldsNullable(fieldName, fieldNames, fields)) {
args.push(
`${t.toBindingIdentifierName(fieldName)}${
isNullable(field) ? "?:" : ":"
} ${typeAnnotation}`
);
} else {
args.push(
`${t.toBindingIdentifierName(fieldName)}: ${typeAnnotation}${
isNullable(field) ? " | undefined" : ""
}`
);
if (builderNames.includes(fieldName)) {
Copy link
Member

Choose a reason for hiding this comment

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

Wouldn't it be better to directly iterate over builderNames instead of fieldNames? Since fieldNames is generated from an object, I think that we risk changing the order of its keys without thinking about this problem.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The fieldNames has been sorted according to the orders of buildKeys and anything not in the builders are at tails.

babel/packages/babel-types/scripts/generators/typescript.js

Line 349 in d0a8982

function sortFieldNames(fields, type) {

if (areAllRemainingFieldsNullable(fieldName, builderNames, fields)) {
args.push(
`${t.toBindingIdentifierName(fieldName)}${
isNullable(field) ? "?:" : ":"
} ${typeAnnotation}`
);
} else {
args.push(
`${t.toBindingIdentifierName(fieldName)}: ${typeAnnotation}${
isNullable(field) ? " | undefined" : ""
}`
);
}
}

const alphaNumeric = /^\w+$/;
Expand Down