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

fix: updated field_dropdown to properly infer its Field type with TS 5.3.3 #7939

Merged
merged 3 commits into from
Mar 15, 2024
Merged

fix: updated field_dropdown to properly infer its Field type with TS 5.3.3 #7939

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ef899fe
fix: updated field_dropdown to properly infer its Field type with TS …
btw17 Mar 15, 2024
e206e38
fix: removed undefined as an option as its not needed for the type fix
btw17 Mar 15, 2024
2b29e76
fix: updated field_dropdown to allow |undefined class validation
btw17 Mar 15, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 8 additions & 2 deletions core/field_dropdown.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -408,7 +408,13 @@ export class FieldDropdown extends Field<string> {
* @param newValue The input value.
* @returns A valid language-neutral option, or null if invalid.
*/
protected override doClassValidation_(newValue?: string): string | null {
protected override doClassValidation_(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this a bad diff, or are there three lines in a row declaring the same function? And if it's the latter, why is that necessary and allowed?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is accurate. This is actually how we have it defined in field.ts, too:
https://github.com/google/blockly/blob/master/core/field.ts#L1183

What this does is allow inferring type info based on what you're supplying. Taking Field's doClassValidation as an example, implementation-wise, undefined means to take new value as is instead of transforming it in some way. What happens is if you provide a newValue as a non-undefined value, it infers the first definition that matches.

  // Providing a non-undefined value allows returning `undefined` (which means to take that value as is)
  protected doClassValidation_(newValue: T): T | null | undefined;
  // Providing an undefined or possibly undefined value means you can't return `undefined` since
  // that would mean take the default, though default could be `undefined` which is not valid to set.
  protected doClassValidation_(newValue?: AnyDuringMigration): T | null;
  // This accepts all the implementations above and we handle the cases in code through this.
  protected doClassValidation_(
    newValue?: T | AnyDuringMigration,
  ): T | null | undefined {
    if (newValue === null || newValue === undefined) {
      return null;
    }

    return newValue as T;
  }

newValue: string,
): string | null | undefined;
Copy link
Member Author

Choose a reason for hiding this comment

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

Just noticed a nitpick - even thoug the base class has | undefined, this class has never expected that and it's actually not needed here. (I confirmed locally.) Updating the slight tweak.

Copy link
Contributor

Choose a reason for hiding this comment

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

class validators are allowed to return undefined: https://developers.google.com/blockly/guides/create-custom-blocks/fields/validators#return_values

I think we need to keep the | undefined to allow subclasses of Dropdown to continue returning undefined

Copy link
Member Author

Choose a reason for hiding this comment

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

Sounds good! I'll make the update.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done!

protected override doClassValidation_(newValue?: string): string | null;
protected override doClassValidation_(
newValue?: string,
): string | null | undefined {
const options = this.getOptions(true);
const isValueValid = options.some((option) => option[1] === newValue);

Expand All @@ -426,7 +432,7 @@ export class FieldDropdown extends Field<string> {
}
return null;
}
return newValue as string;
return newValue;
}

/**
Expand Down
8 changes: 4 additions & 4 deletions package-lock.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,7 @@
"prettier": "3.2.5",
"readline-sync": "^1.4.10",
"rimraf": "^5.0.0",
"typescript": "^5.0.2",
"typescript": "^5.3.3",
"webdriverio": "^8.32.2",
"yargs": "^17.2.1"
},
Expand Down