fix: Fix compilation errors under Closure's strict mode #6073

gonfunko · 2022-04-12T18:21:53Z

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

Progress towards #5857

Proposed Changes

This PR resolves many compilation errors in core when run under Closure's strict mode.

Reason for Changes

Blockly needs to compile cleanly under strict mode before it can be migrated to TS.

Test Coverage

Test suite passes.

rachel-fenichel

Phew! There's a lot here.

core/block_animations.js

rachel-fenichel · 2022-04-12T18:46:14Z

core/block_animations.js

  const percent = ms / DURATION;

  if (percent > 1) {
-    group.skew_ = '';
+    group.setAttribute('data-skew', '');


It looks like you're just storing the skew data on the DOM between steps. Writing to and reading from the DOM can be slow.

Please confirm that this is the same number of writes as the previous code. In future we can clean it up to use local variables instead of storing data on the DOM.

I think we should look for an alternate solution here for another reason as well.

The core issue here is type safety. By setting and accessing random properties on an otherwise well-defined type, we are ignoring type safety. Moving those random properties to a data attribute does not solve the type safety issue, it just moves it to a place where Closure (and eventually TypeScript) cannot yell at us about the issue.

So I would prefer to see an alternate solution such as using a local variable / additional parameter (if this property is not used outside this class), or otherwise defining an actually useful type. In TypeScript there is the intersection type which seems fine for this (see this example of adding the honey property to an Animal: https://www.typescriptlang.org/docs/handbook/2/everyday-types.html#differences-between-type-aliases-and-interfaces).

If we are just going to disable the type safety checking anyway then we might as well just cast to any instead of using the dom to avoid the check. That would at least be trackable / lintable and easier to follow up on later to make improvements.

Thanks for calling this out. This definitely does write to the DOM more, and you're also quite right that this doesn't actually fix things from a type safety perspective. It seems like I could just use a local variable for skew that gets passed to the recursive function call, but the skew also seems to be used when when dragging a block or comment in a workspace without a drag surface. From a cursory glance I think removing it means that the wiggle wouldn't happen on a drag surface-less workspace; perhaps that's enough of an edge case we can live with it? If not I'll probably opt for the any (well, ? for now) cast as you suggested.

Go for any to avoid changes in behaviour, but file an issue to follow up and mark it as a P1 for this quarter.

core/block_dragger.js

core/field_dropdown.js

core/insertion_marker_manager.js

rachel-fenichel · 2022-04-12T20:36:25Z

core/rendered_connection.js

@@ -296,13 +298,15 @@ class RenderedConnection extends Connection {
      // Vertical line, puzzle tab, vertical line.
      const yLen = renderConstants.TAB_OFFSET_FROM_TOP;
      steps = svgPaths.moveBy(0, -yLen) + svgPaths.lineOnAxis('v', yLen) +
-          shape.pathDown + svgPaths.lineOnAxis('v', yLen);
+          (/** @type {PuzzleTab} */ (shape)).pathDown +


This is tricky. It's not guaranteed to be a puzzle tab. It's just guaranteed to have a pathDown property. Same issue with the notch.

rachel-fenichel · 2022-04-12T20:55:53Z

core/workspace_svg.js

@@ -1237,13 +1237,13 @@ class WorkspaceSvg extends Workspace {
      this.cachedParentSvgSize_.width = width;
      // This is set to support the public (but deprecated) Blockly.svgSize
      // method.
-      svg.cachedWidth_ = width;


TODO as a followup: delete svgSize and everything associated. Its deprecation window has passed, but deletion should be a standalone PR.

maribethb

So this is a lot of work and I definitely respect that, so feel free to push back on this feedback.

But in general I would prefer to see fewer casts, and instead choose one of the following as appropriate

Better type choice from the beginning (like you already did by changing Element to HTMLElement, etc) or at the source
More type refinement checks instead (check if the thing is an instanceof another thing, then Closure and TypeScript compilers both know how to handle that smartly with no casts)
some special cases could probably use more general improvements, like checking the type instead of using isCollapsible, but i think those cases are more for improving in the future rather than changing here.

casts will still be necessary and fine in some cases (e.g. the document.createElement calls that can't statically return the correct / more narrow type) but there are others where I think we could improve, though again I acknowledge that is more work and probably requires digging in on a more case-by-case basis.

also I didn't catch all of them but all of the casts to non-primitive types should have nullability modifiers. i think we must have a setting turned off to do strict null checks because otherwise closure should definitely complain when you cast something to implicitly nullable (with no modifier) and then immediately assume it's actually not null. but i think this will cause problems because (hopefully) TypeScript will yell about that, as it should.

maribethb · 2022-04-12T23:20:23Z

core/block_animations.js

  const percent = ms / DURATION;

  if (percent > 1) {
-    group.skew_ = '';
+    group.setAttribute('data-skew', '');


I think we should look for an alternate solution here for another reason as well.

The core issue here is type safety. By setting and accessing random properties on an otherwise well-defined type, we are ignoring type safety. Moving those random properties to a data attribute does not solve the type safety issue, it just moves it to a place where Closure (and eventually TypeScript) cannot yell at us about the issue.

So I would prefer to see an alternate solution such as using a local variable / additional parameter (if this property is not used outside this class), or otherwise defining an actually useful type. In TypeScript there is the intersection type which seems fine for this (see this example of adding the honey property to an Animal: https://www.typescriptlang.org/docs/handbook/2/everyday-types.html#differences-between-type-aliases-and-interfaces).

If we are just going to disable the type safety checking anyway then we might as well just cast to any instead of using the dom to avoid the check. That would at least be trackable / lintable and easier to follow up on later to make improvements.

core/browser_events.js

core/bubble.js

maribethb · 2022-04-12T23:47:47Z

core/bubble_dragger.js

-    if (this.draggingBubble_.isComment) {
-      // TODO (adodson): Resolve build errors when requiring
-      // WorkspaceCommentSvg.
+    if (this.draggingBubble_ instanceof WorkspaceCommentSvg) {
      const event = /** @type {!CommentMove} */
          (new (eventUtils.get(eventUtils.COMMENT_MOVE))(
              /** @type {!WorkspaceCommentSvg} */ (this.draggingBubble_)));


I don't think you should need this cast either inside your new type refinement check. also it's weird to me that (new (eventUtils.get(eventUtils.COMMENT_MOVE)) would need to be cast... why doesn't that just return the correct type?

Done, thanks! The CommentMove cast appears to be necessary because eventUtils.get returns the Abstract event class. I agree it would be nice if it used generics but that'd be a breaking change and probably better suited for post-TS cleanup?

We can eventually do something like this: https://www.typescriptlang.org/docs/handbook/dom-manipulation.html#documentcreateelement

core/clipboard.js

core/renderers/common/drawer.js

maribethb · 2022-04-13T01:11:41Z

core/renderers/common/drawer.js

@@ -186,7 +191,7 @@ class Drawer {
    this.positionExternalValueConnection_(row);

    const pathDown = (typeof input.shape.pathDown === 'function') ?
-        input.shape.pathDown(input.height) :
+        (/** @type {function(number)} */ (input.shape.pathDown))(input.height) :


this one feels weird to me too. i don't feel i've seen a typecast just so we can tell it what type the parameter will be. seems like closure knowing it's a function should be enough but i guess not.

core/toolbox/category.js

core/toolbox/toolbox.js

core/xml.js

rachel-fenichel · 2022-04-13T17:22:03Z

@maribethb -- I agree with your points where we can use instanceof and rely on type narrowing.

Where larger changes are required, including for type safety, I prefer to file issues and mark them as P1 for this quarter.

The internal typescript migration guide says:

Preserve runtime behavior.

Be aware that the .js files returned after TypeScript compilation aren't guaranteed to be a character-for-character equal to the original – though they should behave the same. If you create .ts files during the migration that violate that original intent, you would be violating the safety of the migration.

Don't fix bugs or do cleanups during the migration.

A TypeScript migration forces you to examine every single file in your codebase. In each file, you'll touch 30% to 50% of the lines. Though you'll likely see issues that you want to fix, don't clean up things now.

Fixing bugs during migration violates the "preserve runtime behavior" principle. Most migrations are done on codebases that are already in production and working adequately well.

Suppressing new compiler errors is OK.

The TypeScript compiler is often stricter. Unfortunately, proper fixes for new errors often means different runtime behavior. Instead of proper fixes, choose suppressions like go/any_during_migration. Make sure that the suppressions are easy to search and find so they can be properly fixed after the TypeScript migration.

If Aaron can get these errors out of the way, it lets him find the next blocker for migration, and we need to find those as early as possible. I can follow up and do cleanup in separate PRs--which also makes it easier to undo if the cleanup changes runtime behaviour and causes bugs.

…umented fields.

…zzleTab/Notch.

rachel-fenichel

This looks better. Please add ! to types in zelos/drawer.js, measurables/inline_input.js, common/marker_svg.js, common/drawer.js, and zelos/info.js.

core/flyout_base.js

gonfunko added 2 commits April 12, 2022 18:18

fix: Fix errors under strict compilation.

e89fade

fix: Fix tests that referenced properties converted to data attributes.

2281efe

gonfunko requested a review from a team as a code owner April 12, 2022 18:21

gonfunko requested a review from rachel-fenichel April 12, 2022 18:21

gonfunko assigned rachel-fenichel Apr 12, 2022

gonfunko changed the title ~~Strict cleanup~~ fix: Fix compilation errors under Closure's strict mode Apr 12, 2022

rachel-fenichel reviewed Apr 12, 2022

View reviewed changes

maribethb reviewed Apr 13, 2022

View reviewed changes

gonfunko added 4 commits April 13, 2022 19:22

fix: Incorporate feedback on resolving compiler errors.

62a6197

refactor: Revert changes to skew and translate attributes.

a8f323f

refactor: Introduce LegacyContextMenuOption type to correspond to doc…

03d9f75

…umented fields.

refactor: Introduce PathLeftShape and PathDownShape vs casting to Pu…

4a684e2

…zzleTab/Notch.

rachel-fenichel reviewed Apr 18, 2022

View reviewed changes

core/flyout_base.js Outdated Show resolved Hide resolved

gonfunko added 3 commits April 19, 2022 20:38

chore: Added nullability modifiers to type annotations.

e876c79

refactor: Export FlyoutItem directly.

50703bc

chore: clang-format renderers/zelos/drawer.js.

8468c6f

rachel-fenichel approved these changes Apr 19, 2022

View reviewed changes

gonfunko merged commit edc2a5c into google:develop Apr 19, 2022

gonfunko deleted the strict-cleanup branch April 19, 2022 21:17

release-please bot mentioned this pull request Apr 21, 2022

chore(develop): release blockly 9.0.0 #6096

Closed

rachel-fenichel mentioned this pull request Apr 21, 2022

Additional type and cast cleanups #6097

Open

11 tasks

This was referenced Apr 21, 2022

chore(develop): release blockly 9.0.0 #6100

Closed

chore(develop): release blockly 9.0.0 #6101

Closed

BeksOmega mentioned this pull request Dec 13, 2022

fix: not being able to set field values to empty #6702

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Fix compilation errors under Closure's strict mode #6073

fix: Fix compilation errors under Closure's strict mode #6073

gonfunko commented Apr 12, 2022

rachel-fenichel left a comment

rachel-fenichel Apr 12, 2022

maribethb Apr 12, 2022

gonfunko Apr 13, 2022

rachel-fenichel Apr 13, 2022

rachel-fenichel Apr 12, 2022

rachel-fenichel Apr 12, 2022

maribethb left a comment

maribethb Apr 12, 2022

maribethb Apr 12, 2022

gonfunko Apr 13, 2022

rachel-fenichel Apr 13, 2022

maribethb Apr 13, 2022

rachel-fenichel commented Apr 13, 2022

Preserve runtime behavior.

Don't fix bugs or do cleanups during the migration.

Suppressing new compiler errors is OK.

rachel-fenichel left a comment

fix: Fix compilation errors under Closure's strict mode #6073

fix: Fix compilation errors under Closure's strict mode #6073

Conversation

gonfunko commented Apr 12, 2022

The basics

The details

Resolves

Proposed Changes

Reason for Changes

Test Coverage

rachel-fenichel 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maribethb 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

Choose a reason for hiding this comment

rachel-fenichel commented Apr 13, 2022

Preserve runtime behavior.

Don't fix bugs or do cleanups during the migration.

Suppressing new compiler errors is OK.

rachel-fenichel left a comment

Choose a reason for hiding this comment