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
fix: generated builder parameter should respect builder keys #11002
Conversation
isNullable(field) ? " | undefined" : "" | ||
}` | ||
); | ||
if (builderNames.includes(fieldName)) { |
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.
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.
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.
The fieldNames
has been sorted according to the orders of buildKeys
and anything not in the builders
are at tails.
function sortFieldNames(fields, type) { |
This PR fixes a regression introduced in #10917.
In https://github.com/babel/babel/pull/10917/files#diff-dd23409d66e27b24936f7ae4e316073eR265, the
optional
meta-field is added only if thedefault
meta-field presents. This change makes sense but unexpectedly it breaksbabel/packages/babel-types/scripts/generators/typescript.js
Line 336 in d0a8982
which relies on
optional
to detect if it is a nullable key, so the non-builder fields are marked as non-optional and included in the definition. For example, here is whatProgram
definitionbabel/packages/babel-types/src/definitions/core.js
Line 642 in d0a8982
generates in different versions.
v7.7.4
v7.8.0
The
sourceFile
is not nullable so all the parameters beforesourceFile
becomes non optional parameters. This is a breaking change for types consumers.The problem here is that we should not include
sourceFile
at all because it is not defined inbuilders
and we are really lucky that nobody ever complains that the fifth argumentsourceFile?
does not work at all.This PR filter the args against
BUILDER_KEYS
so the generated types becomewhere the
sourceFile
is removed from builder arguments definition, technically it is a breaking change but practically it should not break anyone because the builder will check the arguments length at runtime.In the long term we should add the generated
index.js.flow
andindex.d.ts
to the codebase so we can track such changes.