chore: added type information to `core/field_dropdown.ts` #6550

btw17 · 2022-10-14T23:42:57Z

The basics

I branched from develop
My pull request is against develop
My code follows the style guide
I ran npm run format and npm run lint

The details

Resolves

Proposed Changes

Provided type information in place of AnyDuringMigration for core/field_dropdown.ts

Behavior Before Change

Less type information.

Behavior After Change

More type information.

Reason for Changes

Cleaning up types that weren't included in the TS migration.

Test Coverage

N/A

Documentation

Yes - just the automatically generated documentation that utilizes the TS types.

Additional Information

I added comments to the file changes with anything of note. There are a couple of spots where the implementation of a few functions has been updated, though it shouldn't impact the functionality.

core/field_dropdown.ts

btw17 · 2022-10-14T23:53:44Z

core/field_dropdown.ts

+    if (isMenuGenerator(menuGenerator)) {
+      if (Array.isArray(menuGenerator)) {
+        validateOptions(menuGenerator);
+        const trimmed = trimOptions(menuGenerator);


Before, trimOptions was mutating the deep copied menuGenerator and, when no ImageProperties labels were provided, setting a new value for menuGenerator_. Assuming there's not an edge case I'm missing where the mutation is needed, this update shifts setting the value for menuGenerator_ to one point, and it should eliminate the need for a deep copy.

btw17 · 2022-10-14T23:55:03Z

core/field_dropdown.ts

-      // is not assignable to parameter of type 'Node'.
-      this.getTextElement().insertBefore(
-          this.arrow_ as AnyDuringMigration, this.textContent_);
+      this.getTextElement().insertBefore(this.arrow_, this.textContent_);


this.arrow_ apparently changed at some point since being given AnyDuringMigration since it worked when I just removed AnyDuringMigration.

btw17 · 2022-10-14T23:59:33Z

Looks like there were a few commits from the master branch I pulled over into this PR that weren't in develop. I'm assuming they'll be merged into develop in a bit, at which point I'll clean up this PR.

maribethb · 2022-10-15T00:17:26Z

Looks like there were a few commits from the master branch I pulled over into this PR that weren't in develop. I'm assuming they'll be merged into develop in a bit, at which point I'll clean up this PR.

Unfortunately this won't be a timely assumption, likely until our next release. It would be best to rebase onto develop.

btw17 · 2022-10-17T22:34:51Z

Looks like there were a few commits from the master branch I pulled over into this PR that weren't in develop. I'm assuming they'll be merged into develop in a bit, at which point I'll clean up this PR.

Unfortunately this won't be a timely assumption, likely until our next release. It would be best to rebase onto develop.

If develop won't be until the next major release, would it be fine to merge this into master instead of develop (once commits are cleaned up accordingly)? The idea is there isn't a noticeable functional change here - just immediately available type updates for the next minor release.

maribethb · 2022-10-17T22:47:01Z

would it be fine to merge this into master instead of develop (once commits are cleaned up accordingly)?

Unfortunately no, that's just not how our workflow works. All PRs should go into develop as that is what we test with before merging a rollup of changes into master. Generally, master is equivalent with whatever code is currently on npm. Pushing something straight to master would break that assumption.

If develop won't be until the next major release,

master won't be merged into develop until the next release (major, minor, or patch*). That is another reason we can't push to master, because everyone else is working from develop. Someone else could change this line in develop, creating a merge conflict at the time of our next release.

* We don't always merge master back into develop when doing a cherry-pick patch release, but my point is that it's not just during major releases.

core/field_dropdown.ts

maribethb · 2022-10-19T01:12:29Z

core/field_dropdown.ts

+      menuGenerator: MenuGenerator, opt_validator?: Function,
+      opt_config?: FieldConfig);
+  constructor(menuGenerator: Sentinel);
+  constructor(


this one seems redundant with the first one. is there a reason it's not?

For function overloading in TS, the final function's parameters need to support all of the previous functions' parameters. The final constructor supports MenuGenerator|Sentinel since those are the cases that need to be handled in code within the constructor. It's also ultimately the limit of what can be provided, so a user could provide both a menuGenerator: Sentinel and a validator.

The first two constructors here just guide what combinations should be provided. Without providing any parameters, it'll suggest providing a MenuGenerator. If a Sentinel is provided, it'll suggest not providing a validator or config. Since Sentinel is only for subclasses, it seems like most users won't want to provide it.

I think this combination of constructors makes sense!

core/field_dropdown.ts

maribethb · 2022-10-19T02:03:52Z

core/field_dropdown.ts

+      (
+          // One or more strings will entirely vanish if we proceed.  Abort.
+          shortest <= prefixLength + suffixLength))
+    return {options: stringOptions};


please add curly braces here; imo we should only skip them if the condition fits on one line, though I don't know if we have a written policy on this.

plus if you have braces, you can move the comment so it's not in the middle of the condition, and clang might give you something prettier.

Will do! I think originally I had the return on line 714, though the formatter probably bumped it to a new line.

Could you still move the comment so that which part is the predicate is a bit easier to parse? I think either put it right above the if-statement, or right about the return statement.

core/field_dropdown.ts

maribethb · 2022-10-20T20:28:57Z

core/field_dropdown.ts

+        validateOptions(menuGenerator);
+        const trimmed = trimOptions(menuGenerator);
+        this.menuGenerator_ = trimmed.options;
+        this.prefixField = trimmed.prefix || null;


if you have trimOptions return null instead of undefined for a nonexistent prefix/suffix you could simplify this. or alternatively this.prefixField could be changed to be optional instead of string|null. I don't love the inconsistency between null and undefined here, though I recognize this problem is endemic to Blockly and is not a new issue here.

Originally, I tried keeping the resulting values identical to the previous implementation in mind of a minor release. Since this will probably be included in a major release, though, I think it might be worth making prefixField and suffixField accept undefined instead of null. null is in a weird spot where it's mostly symbolic now, perhaps conveying an explicit choice to provide a non-value rather than choosing to not provide a value.

I'll make the update in favor of undefined since it keeps the code more concise, though I can adjust either way.

Actually, updating prefixField and suffixField would require updating the Field parent class and impacted files so that'd be a pretty expansive change. Would it be fine to leave this as is for now? Ultimately I think those values probably should be just optional strings rather than string|null, though that's probably for a future PR.

btw17 · 2022-10-21T19:46:28Z

I made most of the requested updates and cleaned up the branch to no longer include those commits from master. The only thing left outstanding in the discussion of null vs undefined and if there was any further thoughts about overloading the constructor.

BeksOmega

LGTM! I wrote down a few nits, but they're all optional in my mind, so I'm going ahead and approving this =) Will merge Tuesday after making sure Maribeth is cool with it.

core/field_dropdown.ts

BeksOmega · 2022-10-21T21:10:39Z

core/field_dropdown.ts

+      (
+          // One or more strings will entirely vanish if we proceed.  Abort.
+          shortest <= prefixLength + suffixLength))
+    return {options: stringOptions};


Could you still move the comment so that which part is the predicate is a bit easier to parse? I think either put it right above the if-statement, or right about the return statement.

core/field_dropdown.ts

BeksOmega · 2022-10-21T21:17:43Z

core/field_variable.ts

-    // AnyDuringMigration because:  Type '(this: FieldVariable) => any[][]' is
-    // not assignable to type 'any[][] | ((this: FieldDropdown) => any[][])'.
-    this.menuGenerator_ = FieldVariable.dropdownCreate as AnyDuringMigration;
+    this.menuGenerator_ = FieldVariable.dropdownCreate as MenuGenerator;


Mmm this is kind of unfortunate (having to cast with as), but I think this is the right thing to do here, b/c sadly expecting a FieldVariable means it isn't actually a subtype :/

Yeah, that was my thought as well. I explored a few different options, like updating dropdownCreate to accept generics, though in the end this made the most sense.

Ideally this could be refactored such that this isn't a parameter for the base function, though my goal was to avoid impacting the usage of each function.

BeksOmega · 2022-10-21T21:18:11Z

core/field_dropdown.ts

+      menuGenerator: MenuGenerator, opt_validator?: Function,
+      opt_config?: FieldConfig);
+  constructor(menuGenerator: Sentinel);
+  constructor(


I think this combination of constructors makes sense!

…ffected files

btw17 · 2022-10-21T22:11:29Z

Pulled in this week's develop changes and rebased - should be good to go now!

maribethb

Thanks for making the changes and thanks to Beka for the additional review!

btw17 requested a review from a team as a code owner October 14, 2022 23:42

btw17 requested a review from maribethb October 14, 2022 23:42

github-actions bot assigned maribethb Oct 14, 2022

github-actions bot added the PR: chore General chores (dependencies, typos, etc) label Oct 14, 2022

btw17 commented Oct 14, 2022

View reviewed changes

core/field_dropdown.ts Outdated Show resolved Hide resolved

btw17 commented Oct 14, 2022

View reviewed changes

github-actions bot added PR: chore General chores (dependencies, typos, etc) and removed PR: chore General chores (dependencies, typos, etc) labels Oct 15, 2022

maribethb reviewed Oct 20, 2022

View reviewed changes

btw17 force-pushed the chore/field-dropdown-types branch from 1cd07df to a495529 Compare October 21, 2022 19:35

BeksOmega approved these changes Oct 21, 2022

View reviewed changes

btw17 added 6 commits October 21, 2022 21:57

chore: added generator type to core/field_dropdown.ts and updated a…

b2962ba

…ffected files

chore: added type to ARROW_CHAR in core/field_dropdown.ts

9654b82

chore: cleaned up misc 'AnyDuringMigration' cases and related comments

36a9b39

fix: misc adjustments from PR feedback

012243c

Fix: simplified getOptions

aa784ae

fix: removed outdated arrow and cleaned up formatting

aa007e3

btw17 force-pushed the chore/field-dropdown-types branch from d26e782 to aa007e3 Compare October 21, 2022 22:04

fix: cleanup format after rebase

90044f5

maribethb approved these changes Oct 24, 2022

View reviewed changes

BeksOmega merged commit f22303b into google:develop Oct 24, 2022

btw17 deleted the chore/field-dropdown-types branch October 25, 2022 12:21

cpcallen mentioned this pull request Mar 23, 2023

fix(fields)!: Make Field.SKIP_SETUP a Symbol; remove class Sentinel #6919

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: added type information to `core/field_dropdown.ts` #6550

chore: added type information to `core/field_dropdown.ts` #6550

btw17 commented Oct 14, 2022 •

edited

Loading

btw17 Oct 14, 2022

btw17 Oct 14, 2022

btw17 commented Oct 14, 2022

maribethb commented Oct 15, 2022

btw17 commented Oct 17, 2022 •

edited

Loading

maribethb commented Oct 17, 2022 •

edited

Loading

maribethb Oct 19, 2022

btw17 Oct 21, 2022

BeksOmega Oct 21, 2022

maribethb Oct 19, 2022

btw17 Oct 21, 2022

btw17 Oct 21, 2022

BeksOmega Oct 21, 2022

maribethb Oct 20, 2022

btw17 Oct 21, 2022

btw17 Oct 21, 2022

btw17 commented Oct 21, 2022

BeksOmega left a comment

BeksOmega Oct 21, 2022

BeksOmega Oct 21, 2022

btw17 Oct 21, 2022

BeksOmega Oct 21, 2022

btw17 commented Oct 21, 2022

maribethb left a comment

chore: added type information to core/field_dropdown.ts #6550

chore: added type information to core/field_dropdown.ts #6550

Conversation

btw17 commented Oct 14, 2022 • edited Loading

The basics

The details

Resolves

Proposed Changes

Behavior Before Change

Behavior After Change

Reason for Changes

Test Coverage

Documentation

Additional Information

Choose a reason for hiding this comment

Choose a reason for hiding this comment

btw17 commented Oct 14, 2022

maribethb commented Oct 15, 2022

btw17 commented Oct 17, 2022 • edited Loading

maribethb commented Oct 17, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

btw17 commented Oct 21, 2022

BeksOmega left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

btw17 commented Oct 21, 2022

maribethb left a comment

Choose a reason for hiding this comment

chore: added type information to `core/field_dropdown.ts` #6550

chore: added type information to `core/field_dropdown.ts` #6550

btw17 commented Oct 14, 2022 •

edited

Loading

btw17 commented Oct 17, 2022 •

edited

Loading

maribethb commented Oct 17, 2022 •

edited

Loading