chore: cleaned up several type cases in core/field.ts and impacted files #6363

btw17 · 2022-08-19T23:03:51Z

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

This resolves several AnyDuringMigration type cases in core/field.ts.

Proposed Changes

Updates nullable types, when set, to be <OriginalType>|null to properly reflect its possible state
Updates usage of those nullable types to assert they're available in core/field.ts and impacted descendants
- Note: Used the non-null assertion operator, someValue!.nowExists
Added type information for callingClass which uses Field's prototype
Updates numbers which are expected to be strings to be cast to strings in core/field.ts

Behavior Before Change

Less precise type information.
SVGTextElement.setAttribute passed in numbers as attribute values instead of strings in core/field.ts

Behavior After Change

More precise type information.
SVGTextElement.setAttribute always passes in strings now in core/field.ts

Reason for Changes

Simplifying the information space by offloading type juggling to the compiler.

BeksOmega

LGTM! It is frustrating that so many of these things have to be nullable, but I think that's a problem with the design of the field, not with this change.

One question: How would you feel about changing instances of sourceBlock_! to use getSourceBlock() instead (which already includes the assertion).

btw17 · 2022-08-24T21:38:41Z

One question: How would you feel about changing instances of sourceBlock_! to use getSourceBlock() instead (which already includes the assertion).

I'm 100% for it! With that in mind, can I update sourceBlock_ from protected to private as well? It would force those inheriting from field to use getSourceBlock, though since sourceBlock_ isn't expected to be null most of the time anyway, it could save with some confusion on handling the null case. Edit: there are some existing cases, such as in field_variable.ts in getVariableTypes_, that utilize the base sourceBlock_ and check for null, so I'll leave it protected and just update the sourceBlock_! cases as you mentioned.

btw17 · 2022-08-24T22:29:23Z

@BeksOmega all set! I updated fieldGroup_! to use getSvgRoot() as well. They're split into separate commits, so it should be pretty straightforward to verify.

BeksOmega

Thanks for your work on this dude!

core/field.ts

core/field_angle.ts

BeksOmega

Rebase/merge (your preference) to resolve conflicts, then I think this is gtg.

btw17 · 2022-08-26T16:48:19Z

My tests are broken locally even at origin/develop so I'm not able to verify this locally, though it looks like the tests are running into an issue with getSvgRoot throwing an error for falsy values of fieldGroup_. I'm assuming that could be fixed by reverting getSvgRoot to return this.fieldGroup_!, though this might be more of a sign that getSvgRoot should actually be returning SVGElement|null. What do you think @BeksOmega?

btw17 · 2022-08-26T20:08:17Z

core/field_angle.ts

@@ -202,7 +202,7 @@ export class FieldAngle extends FieldTextInput {
    // #2380)
    this.symbol_ = dom.createSvgElement(Svg.TSPAN, {});
    this.symbol_.appendChild(document.createTextNode('\u00B0'));
-    this.textElement_.appendChild(this.symbol_);
+    this.getTextElement().appendChild(this.symbol_);


Hey @maribethb & @BeksOmega - I went ahead and created getTextElement and other wrapper functions to handle encapsulating the null error check. What do you think about utilizing it in this context, though? Earlier, it was mentioned using this.textElement_! made sense here since it was just defined in initView. The trade off here I think is 'runtime compute' vs 'developer compute', so to speak 😂

Runtime Compute - In cases like this where the null check is so minor, it would take an extreme number of runs for it to impact runtime performance.

Developer Compute - It might be obvious that after initView the textElement_ should be set, though it's just one less thing a developer has to juggle when looking through this code.

Edit: The main purpose of asking here is the conceptual discussion of how to handle cases like this rather than specifically whether or not I need to correct this case (though I will correct it if this.textElement_! would be preferred!)

I think either option makes sense in situations like this where we never expect developers to be interacting with these properties while they're null =)

We discussed this earlier this week and this is one of those cases where it's acceptable because it's easily verified that textElement is set directly beforehand in initView. The property is set and then used in the same method so there is no room for developer error - compare that to fieldGroup_ where it could be set in multiple places and an error like forgetting to call an init method would result in the value being unexpectedly null.

There is no hard and fast rule I can give you but that's why this particular case the ! is fine, though using the method is also fine.

BeksOmega

Just gonna wait for @maribethb 's quick lgtm as well.

maribethb

in general, I would prefer more descriptive error messages that might give you a hint about what has done wrong like "fieldGroup_ is null. Has this field been initialized?" or "You may need to initialize this field first" or something, but that isn't always easy to tell. so don't necessarily need to change these, just something to think about whenever we add these.

maribethb · 2022-08-26T23:26:24Z

core/field.ts

@@ -1217,19 +1228,20 @@ export abstract class Field implements IASTNodeLocationSvg,
   */
  setMarkerSvg(markerSvg: SVGElement) {
    if (!markerSvg) {


I think the parameter to this function needs to be nullable, otherwise this check doesn't make sense. some of our other setMarkerSvg methods have the same logic and the parameter is nullable there. I wonder if this was created from our closure code where we didn't always use ? consistently

maribethb · 2022-08-26T23:36:28Z

core/field_angle.ts

@@ -202,7 +202,7 @@ export class FieldAngle extends FieldTextInput {
    // #2380)
    this.symbol_ = dom.createSvgElement(Svg.TSPAN, {});
    this.symbol_.appendChild(document.createTextNode('\u00B0'));
-    this.textElement_.appendChild(this.symbol_);
+    this.getTextElement().appendChild(this.symbol_);


We discussed this earlier this week and this is one of those cases where it's acceptable because it's easily verified that textElement is set directly beforehand in initView. The property is set and then used in the same method so there is no room for developer error - compare that to fieldGroup_ where it could be set in multiple places and an error like forgetting to call an init method would result in the value being unexpectedly null.

There is no hard and fast rule I can give you but that's why this particular case the ! is fine, though using the method is also fine.

core/field.ts

BeksOmega · 2022-09-01T19:09:49Z

@btw17 give me a ping when this is ready for another look =)

btw17 · 2022-09-23T17:56:58Z

@btw17 give me a ping when this is ready for another look =)

All set now! Just updated this PR to include the latest updates from the develop branch.

BeksOmega · 2022-09-26T21:43:40Z

LGTM after the build passes! If you want to work on this I'll baby sit it to rerun the CI. But don't feel the need to work on it before Friday.

…ions

btw17 requested a review from a team as a code owner August 19, 2022 23:03

btw17 requested a review from maribethb August 19, 2022 23:03

github-actions bot assigned maribethb Aug 19, 2022

BeksOmega approved these changes Aug 22, 2022

View reviewed changes

btw17 force-pushed the chore/cleanup-any-types branch from ffebb56 to 2138e80 Compare August 24, 2022 22:27

BeksOmega requested changes Aug 24, 2022

View reviewed changes

core/field.ts Show resolved Hide resolved

core/field.ts Outdated Show resolved Hide resolved

core/field.ts Show resolved Hide resolved

core/field_angle.ts Outdated Show resolved Hide resolved

BeksOmega requested review from BeksOmega and removed request for maribethb August 25, 2022 16:22

BeksOmega assigned BeksOmega and unassigned maribethb Aug 25, 2022

github-actions bot assigned maribethb Aug 25, 2022

BeksOmega approved these changes Aug 25, 2022

View reviewed changes

BeksOmega changed the title ~~Chore: cleaned up several type cases in core/field.ts and impacted files~~ chore: cleaned up several type cases in core/field.ts and impacted files Aug 25, 2022

BeksOmega unassigned maribethb Aug 25, 2022

btw17 force-pushed the chore/cleanup-any-types branch from 58a6045 to 9396432 Compare August 26, 2022 14:43

btw17 force-pushed the chore/cleanup-any-types branch from d990457 to b5aac27 Compare August 26, 2022 19:30

btw17 commented Aug 26, 2022

View reviewed changes

btw17 force-pushed the chore/cleanup-any-types branch from b5aac27 to dd10ba4 Compare August 26, 2022 21:03

BeksOmega approved these changes Aug 26, 2022

View reviewed changes

maribethb approved these changes Aug 26, 2022

View reviewed changes

btw17 force-pushed the chore/cleanup-any-types branch from dd10ba4 to 23b49f9 Compare September 23, 2022 17:47

BeksOmega approved these changes Sep 26, 2022

View reviewed changes

btw17 added 2 commits September 30, 2022 15:28

chore: cleaned up several type cases in core/field.ts and impacted files

bb2cea1

chore: updated instances of sourceBlock_! to getSourceBlock()

f25a097

btw17 added 8 commits September 30, 2022 15:28

chore: updated instances of fieldGroup_! to getSvgRoot()

da6099e

chore: updated nullable variables in field.ts to use internal funct…

74aa814

…ions

chore: updated getSourceBlock and getSvgRoot to handle nullability

361c137

chore: updated comments to reference throwing an error

3aac666

fix: reverted getSvgRoot to fieldGroup_ in null-accepting areas

2a937af

fix: updated getSvgRoot to allow null and added null handling methods

acdeb85

fix: moved click target error handling to their specific cases

af4642b

fix: updated drawer.ts to handle cast svg group to defined

d6801da

btw17 force-pushed the chore/cleanup-any-types branch from d98545c to d6801da Compare September 30, 2022 15:42

BeksOmega merged commit fa925e8 into google:develop Sep 30, 2022

btw17 deleted the chore/cleanup-any-types branch September 30, 2022 15:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: cleaned up several type cases in core/field.ts and impacted files #6363

chore: cleaned up several type cases in core/field.ts and impacted files #6363

btw17 commented Aug 19, 2022 •

edited

BeksOmega left a comment

btw17 commented Aug 24, 2022 •

edited

btw17 commented Aug 24, 2022

BeksOmega left a comment

BeksOmega left a comment

btw17 commented Aug 26, 2022

btw17 Aug 26, 2022 •

edited

BeksOmega Aug 26, 2022

maribethb Aug 26, 2022

BeksOmega left a comment

maribethb left a comment

maribethb Aug 26, 2022

maribethb Aug 26, 2022

BeksOmega commented Sep 1, 2022

btw17 commented Sep 23, 2022

BeksOmega commented Sep 26, 2022

chore: cleaned up several type cases in core/field.ts and impacted files #6363

chore: cleaned up several type cases in core/field.ts and impacted files #6363

Conversation

btw17 commented Aug 19, 2022 • edited

The basics

The details

Resolves

Proposed Changes

Behavior Before Change

Behavior After Change

Reason for Changes

BeksOmega left a comment

Choose a reason for hiding this comment

btw17 commented Aug 24, 2022 • edited

btw17 commented Aug 24, 2022

BeksOmega left a comment

Choose a reason for hiding this comment

BeksOmega left a comment

Choose a reason for hiding this comment

btw17 commented Aug 26, 2022

btw17 Aug 26, 2022 • edited

Choose a reason for hiding this comment

BeksOmega Aug 26, 2022

Choose a reason for hiding this comment

maribethb Aug 26, 2022

Choose a reason for hiding this comment

BeksOmega left a comment

Choose a reason for hiding this comment

maribethb left a comment

Choose a reason for hiding this comment

maribethb Aug 26, 2022

Choose a reason for hiding this comment

maribethb Aug 26, 2022

Choose a reason for hiding this comment

BeksOmega commented Sep 1, 2022

btw17 commented Sep 23, 2022

BeksOmega commented Sep 26, 2022

btw17 commented Aug 19, 2022 •

edited

btw17 commented Aug 24, 2022 •

edited

btw17 Aug 26, 2022 •

edited