Executing a deeper penetration into Google's open-source project

Issues

1. Enable strict mode for workspace-search
2. Enable strict mode for workspace-backpack

Pull Requests

1. feature: Enable strict mode for workspace search #2032
2. feature: enabled TS strict mode in workspace-backapck, resolved TS errors

Issue scope

Both the above issues I worked on dealt with 2 different parts of the project.

• workspace-search plugin - allows a user to search for items present within the blocks on the current workspace. An interactive sample can be found here. Looks like this:

• workspace-backpack plugin - allows a user to save current blocks on the workspace to a backpack. An interactive sample can be found here. Looks like this:

Instructions

The instructions on how to work and the testing to be performed for these issues were the same for both of them.

1. Run npm install at the root of Blockly samples to set up dependencies.
2. Navigate to the plugin's directory
3. Change strict: false to strict: true in the plugin's tsconfig.json
4. Run npm run build and see if there are any TypeScript errors.
5. Resolve any TypeScript errors, if present. If not, then we got lucky and accidentally wrote strict code! Yay!
6. Run npm run build again to make sure there are no TypeScript errors.
7. Run npm run start and play with the plugin in the playground to make sure the plugin still behaves as expected.
8. Run npm run test to run any automated tests.
9. Run npm run lint to make sure there are no formatting errors.
10. Open a PR with your changes!

TS error list

1. Received 16 TS errors for Enable strict mode for workspace-search issue, after turning the strict flag from false to true. The errors are listed below:

[tsl] ERROR in blockly-samples/plugins/workspace-search/src/workspace_search.ts(136,50)
      TS2345: Argument of type '(evt: KeyboardEvent) => void' is not assignable to parameter of type '(event: Event) => void'.
  Types of parameters 'evt' and 'event' are incompatible.
    Type 'Event' is missing the following properties from type 'KeyboardEvent': altKey, charCode, code, ctrlKey, and 17 more.

[tsl] ERROR in blockly-samples/plugins/workspace-search/src/workspace_search.ts(153,55)
      TS2345: Argument of type '(evt: KeyboardEvent) => void' is not assignable to parameter of type '(event: Event) => void'.
  Types of parameters 'evt' and 'event' are incompatible.
    Type 'Event' is not assignable to type 'KeyboardEvent'.

[tsl] ERROR in blockly-samples/plugins/workspace-search/src/workspace_search.ts(159,7)
      TS2531: Object is possibly 'null'.

[tsl] ERROR in blockly-samples/plugins/workspace-search/src/workspace_search.ts(223,5)
      TS2531: Object is possibly 'null'.

[tsl] ERROR in blockly-samples/plugins/workspace-search/src/workspace_search.ts(294,41)
      TS2345: Argument of type '(e: KeyboardEvent) => void' is not assignable to parameter of type '(event: Event) => void'.
  Types of parameters 'e' and 'event' are incompatible.
    Type 'Event' is not assignable to type 'KeyboardEvent'.

[tsl] ERROR in blockly-samples/plugins/workspace-search/src/workspace_search.ts(331,7)
      TS2531: Object is possibly 'null'.

[tsl] ERROR in blockly-samples/plugins/workspace-search/src/workspace_search.ts(334,9)
      TS2531: Object is possibly 'null'.

[tsl] ERROR in blockly-samples/plugins/workspace-search/src/workspace_search.ts(336,9)
      TS2531: Object is possibly 'null'.

[tsl] ERROR in blockly-samples/plugins/workspace-search/src/workspace_search.ts(339,5)
      TS2531: Object is possibly 'null'.

[tsl] ERROR in blockly-samples/plugins/workspace-search/src/workspace_search.ts(347,26)
      TS2531: Object is possibly 'null'.

[tsl] ERROR in blockly-samples/plugins/workspace-search/src/workspace_search.ts(366,28)
      TS2531: Object is possibly 'null'.

[tsl] ERROR in blockly-samples/plugins/workspace-search/src/workspace_search.ts(440,5)
      TS2531: Object is possibly 'null'.

[tsl] ERROR in blockly-samples/plugins/workspace-search/src/workspace_search.ts(461,5)
      TS2531: Object is possibly 'null'.

[tsl] ERROR in blockly-samples/plugins/workspace-search/src/workspace_search.ts(524,13)
      TS7034: Variable 'topBlockText' implicitly has type 'any[]' in some locations where its type cannot be determined.

[tsl] ERROR in blockly-samples/plugins/workspace-search/src/workspace_search.ts(530,19)
      TS7005: Variable 'topBlockText' implicitly has an 'any[]' type.

[tsl] ERROR in blockly-samples/plugins/workspace-search/test/index.ts(42,22)
      TS2345: Argument of type 'HTMLElement | null' is not assignable to parameter of type 'HTMLElement'.
  Type 'null' is not assignable to type 'HTMLElement'.

1. Received 45 TS errors for Enable strict mode for workspace-backpack issue, after turning the strict flag from false to true. The errors are listed below:

[tsl] ERROR in blockly-samples/plugins/workspace-backpack/src/backpack.ts(139,26)
      TS2345: Argument of type 'BackpackContextMenuOptions | undefined' is not assignable to parameter of type 'BackpackContextMenuOptions'.
  Type 'undefined' is not assignable to type 'BackpackContextMenuOptions'.

[tsl] ERROR in blockly-samples/plugins/workspace-backpack/src/backpack.ts(170,7)
      TS2322: Type '{ [rendererConstant: string]: any; } | null' is not assignable to type '{ [rendererConstant: string]: any; } | undefined'.
  Type 'null' is not assignable to type '{ [rendererConstant: string]: any; } | undefined'.

[tsl] ERROR in blockly-samples/plugins/workspace-backpack/src/backpack.ts(186,26)
      TS18047: 'HorizontalFlyout' is possibly 'null'.

[tsl] ERROR in blockly-samples/plugins/workspace-backpack/src/backpack.ts(197,26)
      TS18047: 'VerticalFlyout' is possibly 'null'.

[tsl] ERROR in blockly-samples/plugins/workspace-backpack/src/backpack.ts(201,5)
      TS18047: 'parentNode' is possibly 'null'.

[tsl] ERROR in blockly-samples/plugins/workspace-backpack/src/backpack.ts(257,7)
      TS2345: Argument of type 'SVGElement | null' is not assignable to parameter of type 'Element'.
  Type 'null' is not assignable to type 'Element'.

[tsl] ERROR in blockly-samples/plugins/workspace-backpack/src/backpack.ts(262,19)
      TS2345: Argument of type 'SVGElement | null' is not assignable to parameter of type 'Element'.
  Type 'null' is not assignable to type 'Element'.

[tsl] ERROR in blockly-samples/plugins/workspace-backpack/src/backpack.ts(263,19)
      TS2345: Argument of type 'SVGElement | null' is not assignable to parameter of type 'Element'.
  Type 'null' is not assignable to type 'Element'.

[tsl] ERROR in blockly-samples/plugins/workspace-backpack/src/backpack.ts(264,19)
      TS2345: Argument of type 'SVGElement | null' is not assignable to parameter of type 'Element'.
  Type 'null' is not assignable to type 'Element'.

[tsl] ERROR in blockly-samples/plugins/workspace-backpack/src/backpack.ts(410,5)
      TS2531: Object is possibly 'null'.

[tsl] ERROR in blockly-samples/plugins/workspace-backpack/src/backpack.ts(465,36)
      TS7006: Parameter 'json' implicitly has an 'any' type.

[tsl] ERROR in blockly-samples/plugins/workspace-backpack/src/backpack.ts(465,42)
      TS7006: Parameter 'keys' implicitly has an 'any' type.

[tsl] ERROR in blockly-samples/plugins/workspace-backpack/src/backpack.ts(624,7)
      TS2531: Object is possibly 'null'.

[tsl] ERROR in blockly-samples/plugins/workspace-backpack/src/backpack.ts(630,7)
      TS2531: Object is possibly 'null'.

[tsl] ERROR in blockly-samples/plugins/workspace-backpack/src/backpack.ts(668,12)
      TS2531: Object is possibly 'null'.

[tsl] ERROR in blockly-samples/plugins/workspace-backpack/src/backpack.ts(679,5)
      TS2531: Object is possibly 'null'.

[tsl] ERROR in blockly-samples/plugins/workspace-backpack/src/backpack.ts(701,5)
      TS2531: Object is possibly 'null'.

[tsl] ERROR in blockly-samples/plugins/workspace-backpack/src/backpack.ts(711,5)
      TS2531: Object is possibly 'null'.

[tsl] ERROR in blockly-samples/plugins/workspace-backpack/src/backpack.ts(803,34)
      TS2345: Argument of type 'SVGImageElement | null' is not assignable to parameter of type 'Element'.
  Type 'null' is not assignable to type 'Element'.

[tsl] ERROR in blockly-samples/plugins/workspace-backpack/src/backpack.ts(805,37)
      TS2345: Argument of type 'SVGImageElement | null' is not assignable to parameter of type 'Element'.
  Type 'null' is not assignable to type 'Element'.

[tsl] ERROR in blockly-samples/plugins/workspace-backpack/src/backpack_helpers.ts(26,3)
      TS2322: Type '(menuOptions: ContextMenuOption[], e: PointerEvent) => void' is not assignable to type '(menuOptions: ContextMenuOption[], e: Event) => void'.
  Types of parameters 'e' and 'e' are incompatible.
    Type 'Event' is missing the following properties from type 'PointerEvent': height, isPrimary, pointerId, pointerType, and 33 more.

[tsl] ERROR in blockly-samples/plugins/workspace-backpack/src/backpack_helpers.ts(30,23)
      TS2531: Object is possibly 'null'.

[tsl] ERROR in blockly-samples/plugins/workspace-backpack/src/backpack_helpers.ts(61,18)
      TS18048: 'scope.block' is possibly 'undefined'.

[tsl] ERROR in blockly-samples/plugins/workspace-backpack/src/backpack_helpers.ts(66,25)
      TS2531: Object is possibly 'null'.

[tsl] ERROR in blockly-samples/plugins/workspace-backpack/src/backpack_helpers.ts(73,24)
      TS18048: 'scope.block' is possibly 'undefined'.

[tsl] ERROR in blockly-samples/plugins/workspace-backpack/src/backpack_helpers.ts(73,24)
      TS18047: 'scope.block.workspace.targetWorkspace' is possibly 'null'.

[tsl] ERROR in blockly-samples/plugins/workspace-backpack/src/backpack_helpers.ts(76,28)
      TS2345: Argument of type 'BlockSvg | undefined' is not assignable to parameter of type 'Block'.
  Type 'undefined' is not assignable to type 'Block'.

[tsl] ERROR in blockly-samples/plugins/workspace-backpack/src/backpack_helpers.ts(108,18)
      TS18048: 'scope.block' is possibly 'undefined'.

[tsl] ERROR in blockly-samples/plugins/workspace-backpack/src/backpack_helpers.ts(117,41)
      TS2345: Argument of type 'BlockSvg | undefined' is not assignable to parameter of type 'Block'.
  Type 'undefined' is not assignable to type 'Block'.

[tsl] ERROR in blockly-samples/plugins/workspace-backpack/src/backpack_helpers.ts(123,24)
      TS18048: 'scope.block' is possibly 'undefined'.

[tsl] ERROR in DPI/blockly-samples/plugins/workspace-backpack/src/backpack_helpers.ts(126,25)
      TS2345: Argument of type 'BlockSvg | undefined' is not assignable to parameter of type 'Block'.
  Type 'undefined' is not assignable to type 'Block'.

[tsl] ERROR in blockly-samples/plugins/workspace-backpack/src/backpack_helpers.ts(133,49)
      TS2345: Argument of type '{ displayText: (scope: Scope) => string | undefined; preconditionFn: (scope: Scope) => "enabled" | "hidden" | "disabled"; callback: (scope: Scope) => void; scopeType: ScopeType; id: string; weight: number; }' is not assignable to parameter of type 'RegistryItem'.
  Types of property 'displayText' are incompatible.
    Type '(scope: Scope) => string | undefined' is not assignable to type 'string | ((p1: Scope) => string)'.
      Type '(scope: Scope) => string | undefined' is not assignable to type '(p1: Scope) => string'.
        Type 'string | undefined' is not assignable to type 'string'.
          Type 'undefined' is not assignable to type 'string'.

[tsl] ERROR in blockly-samples/plugins/workspace-backpack/src/backpack_helpers.ts(148,12)
      TS18048: 'ws' is possibly 'undefined'.

[tsl] ERROR in blockly-samples/plugins/workspace-backpack/src/backpack_helpers.ts(149,26)
      TS18048: 'ws' is possibly 'undefined'.

[tsl] ERROR in blockly-samples/plugins/workspace-backpack/src/backpack_helpers.ts(158,24)
      TS18048: 'ws' is possibly 'undefined'.

[tsl] ERROR in blockly-samples/plugins/workspace-backpack/src/backpack_helpers.ts(161,26)
      TS18048: 'ws' is possibly 'undefined'.

[tsl] ERROR in blockly-samples/plugins/workspace-backpack/src/backpack_helpers.ts(192,12)
      TS18048: 'ws' is possibly 'undefined'.

[tsl] ERROR in blockly-samples/plugins/workspace-backpack/src/backpack_helpers.ts(193,26)
      TS18048: 'ws' is possibly 'undefined'.

[tsl] ERROR in blockly-samples/plugins/workspace-backpack/src/backpack_helpers.ts(202,24)
      TS18048: 'ws' is possibly 'undefined'.

[tsl] ERROR in blockly-samples/plugins/workspace-backpack/src/backpack_helpers.ts(209,11)
      TS2345: Argument of type 'WorkspaceSvg | undefined' is not assignable to parameter of type 'Workspace'.
  Type 'undefined' is not assignable to type 'Workspace'.

[tsl] ERROR in blockly-samples/plugins/workspace-backpack/src/backpack_helpers.ts(219,49)
      TS2345: Argument of type '{ displayText: (scope: Scope) => string | undefined; preconditionFn: (scope: Scope) => "enabled" | "hidden"; callback: (scope: Scope) => void; scopeType: ScopeType; id: string; weight: number; }' is not assignable to parameter of type 'RegistryItem'.
  Types of property 'displayText' are incompatible.
    Type '(scope: Scope) => string | undefined' is not assignable to type 'string | ((p1: Scope) => string)'.
      Type '(scope: Scope) => string | undefined' is not assignable to type '(p1: Scope) => string'.
        Type 'string | undefined' is not assignable to type 'string'.
          Type 'undefined' is not assignable to type 'string'.

[tsl] ERROR in blockly-samples/plugins/workspace-backpack/src/backpack_helpers.ts(239,28)
      TS2345: Argument of type 'boolean | undefined' is not assignable to parameter of type 'boolean'.
  Type 'undefined' is not assignable to type 'boolean'.

[tsl] ERROR in blockly-samples/plugins/workspace-backpack/src/ui_events.ts(50,5)
      TS7053: Element implicitly has an 'any' type because expression of type '"isOpen"' can't be used to index type 'AbstractEventJson'.
  Property 'isOpen' does not exist on type 'AbstractEventJson'.

Execution

The way to go about these issues were to,

1. Fix all the errors without thinking about the code quality.
2. Once all errors are fixed, improve code quality while still testing to make sure the code changes haven't introduced new TS errors or ESLint warnings and haven't affected the behaviour of the plugin.

Working on code quality was my utmost priority for these PRs because they take code quality seriously and I know this because I’ve contributed to this repo before for 0.3 release and also went through a few PRs submitted by others to read the maintainers' reviews.

It took me about 3 days of back-and-forth reading, understanding and implementing code changes before I was able to create a PR for both these issues which included multiple commits on these PRs and a pipeline failure for one of the PR for which I provided a separate commit detailed here. I regularly visited TS documentation and Stack Overflow websites to get a better understanding of the errors.

I was confident about most of the code changes in both these PRs except for a couple of them. I decided to implement the changes as per my understanding and if there's a better way to write that specific implementation, I'd bring them up in the conversation on the PR or in the description of the PR to bring it to their attention.

• For PR feature: Enable strict mode for workspace search #2032 couple of code changes I was not confident about were:

On L153, I received error,

TS2345: Argument of type '(e: KeyboardEvent) => void' is not
assignable to parameter of type '(event: Event) => void'.
  Types of parameters 'e' and 'event' are incompatible.
    Type 'Event' is not assignable to type 'KeyboardEvent'.

which relates to declaration of a function called addEvent on L198-203. As the error mentions, the addEvent function declaration takes a function as an argument, whose argument is of type Event, however when addEvent is called, it takes in the event type as KeyboardEvent. TS isn't able to automatically infer at build time that KeyboardEvent is sub-type of type Event after all so I implemented the function with the use of generics by extending the type T from Event so at run time it can automatically infer that the KeyboardEvent type used is indeed the correct one. I was initially planning to perform a check of if (evt instanceof KeyboardEvent) only then execute the rest of the code but the implementation makes the code inflexible so I went the route of generics. If in future another argument of type Function is added to the addEvent function which takes in a different type of event this code can handle it gracefully since it already extend type Event. If the maintainer proposes a different implementation I'll just go for that.

On L40-43 I received error,

TS2345: Argument of type 'HTMLElement | null' is not
assignable to parameter of type 'HTMLElement'.
  Type 'null' is not assignable to type 'HTMLElement'.

This error has to do with type assignment mismatch where the code document.getElementById('root') would return either a value of type HTMLElement or null. However the createPlayground function expects an argument of type HTMLElement. So I implemented the use of type guards by checking whether the returned value from createPlayground function is an instance of HTMLElement interface at runtime. If it is then execute the rest of the code within if block. The issue description mentioned that they generally avoid type casting like variableName as type so I made use of instanceOf type guard since I also saw it being used elsewhere in the codebase. If the maintainer proposes a different implementation I'll just go for that.

For PR feature: enabled TS strict mode in workspace-backapck, resolved TS errors a code change I was not confident about was:

On L465 I received 2 errors,

TS7006: Parameter 'json' implicitly has an 'any' type.

TS7006: Parameter 'keys' implicitly has an 'any' type.

Adding any as both of their types wasn't allowed. So I assigned it a type of Blockly.serialization.blocks.State for the json argument variable and string[] for the keys argument variable. Type assignment for keys worked, however for json, when being used as json[key], another warning popped up,

Element implicitly has an 'any' type because expression
 of type 'string' can't be used to index type 'State'.
  No index signature with a parameter of type 'string'
 was found on type 'State'.ts(7053)

State interface definition

export interface State {
    type: string;
    id?: string;
    x?: number;
    y?: number;
    collapsed?: boolean;
    deletable?: boolean;
    movable?: boolean;
    editable?: boolean;
    enabled?: boolean;
    inline?: boolean;
    data?: string;
    extraState?: any;
    icons?: {
        [key: string]: any;
    };
    fields?: {
        [key: string]: any;
    };
    inputs?: {
        [key: string]: ConnectionState;
    };
    next?: ConnectionState;
}

As per my understanding, what this error means is that TS isn't able to figure out that the usage of json variable in this form, json[key]; to extract out the value of the key is correct. Considering the State interface's properties are keys with different names, TS isn't able to infer that they can be used as strings to get the value of those keys. So I thought of creating a local interface part of this file which extends from State interface whose sole property would be [key: string]: unknown. This property declaration means for an object that represents this interface, there are multiple keys which would be of type string and their values would be of type unknown since we can't figure out what is the type of the value at compile-time and this solved my issue. It resolved all the TS errors plus the ESLint warnings. If the maintainer proposes a different implementation I'll just go for that.

The above examples for both these PRs were the most difficult and least confident out of all for me, but that doesn't mean other code changes were simple and straightforward. I'd have to take constant breaks between the coding session when I was writing code for this PR to process all that I've learnt from reading documentation and my understanding of the codebase to apply that in practise.

I'm hoping for a favourable outcome from these 2 PRs. For all the changes I made for these PRs, I successfully achieved both my goals which were about diving deep into the project's codebase and interfacing closely with TS code.