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.

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

Merged
merged 1 commit into from Jan 13, 2020

Conversation

JLHwung
Copy link
Contributor

@JLHwung JLHwung commented Jan 13, 2020

Q                       A
Fixed Issues? Fixes #10998
Patch: Bug Fix? Yes
Major: Breaking Change? No, see explainers below
Tests Added + Pass? Manual tested
Documentation PR Link
Any Dependency Changes?
License MIT

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 the default meta-field presents. This change makes sense but unexpectedly it breaks

function areAllRemainingFieldsNullable(fieldName, fieldNames, fields) {

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 what Program definition

defineType("Program", {

generates in different versions.

v7.7.4

export function program(
  body: Array<Statement>,
  directives?: Array<Directive>,
  sourceType?: "script" | "module",
  interpreter?: InterpreterDirective | null,
  sourceFile?: string | null
): Program;

v7.8.0

export function program(
  body: Array<Statement>,
  directives: Array<Directive> | undefined,
  sourceType: "script" | "module" | undefined,
  interpreter: InterpreterDirective | null | undefined,
  sourceFile: string
): Program;

The sourceFile is not nullable so all the parameters before sourceFile 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 in builders and we are really lucky that nobody ever complains that the fifth argument sourceFile? does not work at all.

This PR filter the args against BUILDER_KEYS so the generated types become

export function program(
  body: Array<Statement>,
  directives?: Array<Directive>,
  sourceType?: "script" | "module",
  interpreter?: InterpreterDirective | null
): Program;

where 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 and index.d.ts to the codebase so we can track such changes.

@JLHwung JLHwung added PR: Bug Fix 🐛 pkg: types labels Jan 13, 2020
@JLHwung JLHwung requested a review from nicolo-ribaudo Jan 13, 2020
isNullable(field) ? " | undefined" : ""
}`
);
if (builderNames.includes(fieldName)) {
Copy link
Member

@nicolo-ribaudo nicolo-ribaudo Jan 13, 2020

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

@JLHwung JLHwung Jan 13, 2020

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

@nicolo-ribaudo nicolo-ribaudo merged commit 6874c24 into babel:master Jan 13, 2020
4 of 5 checks passed
@nicolo-ribaudo nicolo-ribaudo deleted the fix-10998 branch Jan 13, 2020
@github-actions github-actions bot added the outdated label Apr 14, 2020
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 14, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
i: regression outdated pkg: types PR: Bug Fix 🐛
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants