Delving into open source territory with Google: PR #1
Tags: webdev, javascript, opensource, programming
Published: 2023-11-10 19:49:33

## Project repo:
[blockly-samples](https://github.com/google/blockly-samples) by _Google Open Source_. 

## Project description:
[Blockly](https://developers.google.com/blockly) by _Google Open Source_ is a JavaScript library for building visual programming editor, which looks something like this,
>![blockly sample](https://dev-to-uploads.s3.amazonaws.com/uploads/articles/prrqvx0ssmvxdyhvan7b.png)

The project I worked on is called [blockly-samples](https://github.com/google/blockly-samples) which is a supplementary project to the Blockly project. This project holds code for [Plugins](https://github.com/google/blockly-samples/tree/master/plugins) which are _self-contained pieces of code that add functionality to Blockly. Plugins can add fields, define themes, create renderers, and much more_, [Examples](https://github.com/google/blockly-samples/tree/master/examples) which are _self-contained sample projects demonstrating techniques on how to use Blockly's features and extend the Blockly library_ and [Codelabs](https://github.com/google/blockly-samples/tree/master/codelabs) which are _interactive tutorials demonstrating how to use and customize Blockly._

## Issue
[1978](https://github.com/google/blockly-samples/issues/1978)

## PR
[2065](https://github.com/google/blockly-samples/pull/2065)

## Code Fixes
This issue I worked on for this PR was about fixing 16 ESLint warnings which show up upon running the script `npm run lint` from the root level of the project. Some of them were easy to fix, some not so much. The different warnings I fixed in this PR were,

* Unexpected any. Specify a different type - @typescript-eslint/no-explicit-any
* Missing JSDoc `@param` "type" type - jsdoc/require-param-type
* Missing JSDoc comment - jsdoc/require-jsdoc
* JSDoc @returns declaration present but return expression not available in function - jsdoc/require-returns-check
* Syntax error in type: MigrationData[]                                                                                                      
* Missing JSDoc @returns type  
* Missing JSDoc `@param` "identifier" description - jsdoc/require-param-description
* Missing JSDoc `@param` "identifier" type
* 'Minimap' is defined but never used - @typescript-eslint/no-unused-vars
* JSDoc @returns declaration present but return expression not available in function - jsdoc/require-returns-check

### Example: A couple of easy fixes

>#### Example #1

> One of the warnings I received on line [19](https://github.com/google/blockly-samples/blob/master/plugins/block-dynamic-connection/test/dynamic_if.mocha.js#L19) of `blockly-samples/plugins/block-dynamic-connection/test/dynamic_if.mocha.js` file was `Missing JSDoc @param "type" type jsdoc/require-param-type`. The code before the fix is below,

>>![line 19 of dynamic_if.mocha.js file before fix](https://dev-to-uploads.s3.amazonaws.com/uploads/articles/i142lpukiteb81quthrp.png)

> If we look closely, we see that the order of the parameter named `type` is jumbled up where the tag `@param` is supposed to come first, then comes the type of the parameter `{string=}` followed by the parameter name `type`. So this was quite an easy fix, all I had to do was move the parameter `type` to after the declaration of its type like this,

>>![line 19 of dynamic_if.mocha.js file after fix](https://dev-to-uploads.s3.amazonaws.com/uploads/articles/y8s2wvipes487f3rsg39.png)

>#### Example #2

>Another warning I received was on line [134](https://github.com/google/blockly-samples/blob/master/plugins/block-dynamic-connection/src/dynamic_list_create.ts#L134) of `/blockly-samples/plugins/block-dynamic-connection/src/dynamic_list_create.ts` file was `Unexpected any. Specify a different type  @typescript-eslint/no-explicit-any`. The code before the fix is below,

>>![line 135 of dynamic_list_create.ts file before fix](https://dev-to-uploads.s3.amazonaws.com/uploads/articles/3owsjlajezfsr4exlxi9.png)

>Here the type of the parameter `state` is either an object or a string. The object would contain `string` for its key type and `any` type for its values, which ESLint was not happy about. It was somewhat hard for me to find out the type of the parameter because I tried to find the usage of this function and within that, a piece of code that set the parameter `state` to an object by assigning it a key-value pair, that could've given me the information about the type of the key's value, however, I couldn't find anything of that sort. Then I started reading the code written in the function and noticed that the value of `state['itemCount']` was assigned to another variable called `itemCount`, like this, 

>>![state assignment to itemCount](https://dev-to-uploads.s3.amazonaws.com/uploads/articles/5h6b8y22p8zkrii7toh6.png)

>I looked for `itemCount` declaration in this file and saw that it was initialized like this,

>>![itemCount Declaration](https://dev-to-uploads.s3.amazonaws.com/uploads/articles/2hshvpxu2v0h3ji7d00o.png)

>which immediately gave me the impression that the type of `state`'s object's values would probably be a number because `itemCount` is initialized by a number and also the `count` in the name of the variable `itemCount` gives it up that it'll be of number type. Code after the fix was,

>>![code fix for state](https://dev-to-uploads.s3.amazonaws.com/uploads/articles/u8mb2arkg2d81ue4bq22.png)

### Example: A couple of hard fixes

>#### Example #1

>A warning I was trying to fix was on line [221](https://github.com/google/blockly-samples/blob/master/plugins/field-bitmap/src/field-bitmap.js#L221) of `/blockly-samples/plugins/field-bitmap/src/field-bitmap.js` file about `Missing JSDoc comment jsdoc/require-jsdoc `. A jsdoc comment was missing for a function of [FieldBitmap](https://github.com/google/blockly-samples/tree/master/plugins/field-bitmap) class and I had to add an explanation about the purpose of the function and its return value. Click [here](https://google.github.io/blockly-samples/plugins/field-bitmap/test/index.html) to interact with the `FieldBitmap` plugin. Visually, it looks like this, 

>>![Field-bitmap](https://dev-to-uploads.s3.amazonaws.com/uploads/articles/yjfpumnasyxca9p3mipj.png)

>The code before the fix is below,

>>![missing jsdoc comment](https://dev-to-uploads.s3.amazonaws.com/uploads/articles/nnhl8hiy1pjlbi20h4mx.png)

>So, I started reading and understanding the piece of code written within the function. The code itself wasn't self-explanatory because it was a very small piece of code and a quite technical one.  So I used 2 approaches, one was to read and understand the purpose of the variable `this.fieldGroup_`, the function `getBoundingClientRect()` and the return value `Blockly.utils.Rect(boundingBox.top, boundingBox.bottom, boundingBox.left, boundingBox.right,);` and another approach was to give the Copilot Chat extension in VSCode the above code and make it explain what the code does. The Copilot couldn't help much initially when I gave it the above code so I started reading the code. 

>My hunt to understand the purpose of `getScaledBBox()` is detailed below:

>>* To understand the purpose of the `fieldGroup_`, my hunt took me to Blockly's [Field](https://github.com/google/blockly/blob/develop/core/field.ts) class file where the variable is declared on [L125](https://github.com/google/blockly/blob/develop/core/field.ts#L125) with a type of `SVGGElement | null` since the `getScaledBBox()` function was part of a class called `FieldBitmap` and this class was extending Blockly's `Field` class implementation. Its declaration had a comment on top of it `/** The rendered field's SVG group element. */` which gave me a better visualization and understanding of what this variable represents.

>>* Next I went on a hunt to look for the function `getBoundingClientRect()` which I thought was a member function of `SVGGElement` class since it was being called on the `fieldGroup_` variable. `Alt + left clicking` on the `SVGGElement` type annotation took me to an internal implementation of the class, part of TS's codebase and it had a comment on top of its declaration which took me to the class's [documentation site](https://developer.mozilla.org/en-US/docs/Web/API/SVGGElement). This was just an interface and didn't implement the `getBoundingClientRect()` function I was looking for so I went to its parent interface `SVGGraphicsElement`. 

>>* The class [SVGGraphicsElement](https://developer.mozilla.org/en-US/docs/Web/API/SVGGraphicsElement) implemented [SVGElement](https://developer.mozilla.org/en-US/docs/Web/API/SVGElement) which in turn implemented [Element](https://developer.mozilla.org/en-US/docs/Web/API/Element) which is where I finally found the [getBoundingClientRect()](https://developer.mozilla.org/en-US/docs/Web/API/Element/getBoundingClientRect) implementation. The return value of this function is an SVG's group element's _(an instance of [DOMRect](https://developer.mozilla.org/en-US/docs/Web/API/DOMRect))_ properties like left, top, right, bottom, x, y, width, and height. These describe the position and size of the rectangle in pixels.

>>* The top, right, bottom & left properties are used to re-create a Blockly [Rect](https://github.com/google/blockly/blob/develop/core/utils/rect.ts) and then eventually gets returned from the function `getScaledBBox()`.

>>* This rectangle is used to create the field bitmap as we can see in the image attached above in the current section, showing what it looks like visually. The purpose of the field bitmap is that it lets users input a pixel grid with their mouse. They can paint over the pixels with their mouse by holding the left click and moving the cursor around the grid, or randomize the grid by clicking the `Randomize` button.

>I combined all this information and gave it to the Copilot Chat extension which provided me with good enough information on which I can base my reference, to write the jsdoc comment for the function along with what I learnt through the above process, since at every step I found that the code either had a comment written on top of a declaration be it function or variable to briefly explain its purpose or found a proper documentation site explaining the purpose of the class, in this case mdn web docs, to help me understand what each piece of code means. I used the results from the Copilot and my understanding of the code to put together the jsdoc comment. The code after fix was, 

>>![code after adding comment](https://dev-to-uploads.s3.amazonaws.com/uploads/articles/k6i2y9bgvakaviws36lc.png)

>This was quite a long process to fix this warning and took me quite a lot of time to get it right but I learnt a lot from it.

>#### Example #2
> Another warning that I was trying to fix was 

>>![invalid types error](https://dev-to-uploads.s3.amazonaws.com/uploads/articles/9hb9f5ftza8z9dqhyosw.png)

>for this code,

>>![code](https://dev-to-uploads.s3.amazonaws.com/uploads/articles/94923txztpdt50h6be53.png)

>located in the file `/blockly-samples/plugins/migration/bin/fix-imports.js` part of [keyboard-navigation](https://github.com/google/blockly-samples/tree/master/plugins/keyboard-navigation) plugin. Its interactive demo can be found [here](https://google.github.io/blockly-samples/plugins/keyboard-navigation/test/index.html). The purpose of this plugin is to provide keyboard navigation for better accessibility through a keyboard cursor. You can find its [documentation site here](https://developers.google.com/blockly/guides/configure/web/keyboard-nav) to learn more about it.

> This warning is quite ambiguous. It doesn't explicitly mention what's the issue with the type, it just says the type is invalid. And this issue haunted me for a while to figure out what could possibly be an issue and in the end I had to put out a PR with the issue still unfixed. The issue hasn't been fixed since the PR is still open at the time of writing this blog and have asked the code maintainers to help me with this warning. I took help of AI and the code maintainers to find a fix for it but still couldn't fix it. There's a long discussion that occurred as you can notice on the filed [issue #1978](https://github.com/google/blockly-samples/issues/1978) with code maintainers to try different ways to fix this warning but in the end we decided to file a PR and take up the code changes as a follow-up during the PR review process. I will update the blog about the fix for this issue, if they review my PR and get back to me sooner.

>Some of the tried approaches were:
>>* Removing the tailing comma. The trailing comma is sometimes not allowed due a linting rule.
>>* Changing the type definition, since the current type definition might be deprecated or has errors to something of this sort,
>>```javascript
>>/**
>> * @typedef {Object} MigrationData
>> * @property {string} import
>> * @property {string|undefined} oldIdentifier
>> * @property {string} newIdentifier
>> * @property {string} newImport
>> * @property {string} newRequire
>> */
>>```
>>* Changing name and all its appearances of key named `import` to `importName` since `import` is a reserved keyword in JS/TS.
>>* Optional declaration of key named `oldIdentifier` by changing it to `oldIdentifier: string | undefined` from `oldIdentifier?: string` since optional declaration might not be allowed with ESLint and JSDoc configuration.

>Some of these approaches were suggested by the project maintainers and some were my own implementation based on my research but nothing worked. I also tried to use AI to help me fix this warning but that didn't work either.

>#####N.B.
> There's couple other warnings I fixed in this PR which were hard to deal with and took quite a long time for me to get through them which I haven't mentioned in this blog because I wanted to focus on the key fixes of this PR.

## Code review and feedback

>I received 4 code reviews on my PR. The reviews and the proposed changes are listed below,

>* Received warning: `Syntax error in type: MigrationData[]` on [L62](https://github.com/google/blockly-samples/blob/master/plugins/migration/bin/fix-imports.js#L62). Removed `.` in `Array.<MigrationData>` type declaration which was a quite simple fix i.e., to remove the dot. `Array.<MigrationData>` and `Array<MigrationData>` are different in syntax but are functionally equivalent. Using the dot is an older, non-standard way of declaring generic types so I had to get rid of is since the [reviewer mentioned ](https://github.com/google/blockly-samples/pull/2065#discussion_r1388655692) they don't use the dot syntax anywhere in the codebase.

>* Received warning: `Unexpected any. Specify a different type  @typescript-eslint/no-explicit-any ` on [L41](https://github.com/google/blockly-samples/blob/master/plugins/workspace-minimap/test/index.ts#L41). Removed explicit cast of a function from `createWorkspace as any` to 

>```javascript
    createWorkspace as (
      blocklyDiv: HTMLElement,
      options: Blockly.BlocklyOptions,
    ) => Blockly.WorkspaceSvg
```

> to simply passing it as `createWorkspace` in a function argument. The function is declared in the same file above, before it gets passed as an argument to another function and I failed to check that if the function is passed just by its identifier, removing the original `as any` cast, whether TS infers its type automatically or not. [Reviewer mentioned](https://github.com/google/blockly-samples/pull/2065#discussion_r1390054597) that the full explicit cast is unnecessary and thus I removed it.


>* Received warning: `JSDoc @returns declaration present but return expression not available in function jsdoc/require-returns-check` on [L448](https://github.com/google/blockly-samples/blob/master/scripts/gh-predeploy.js#L448). Restored the changes from `return done();` back to `done(); return;` within a function. The return type of the function was declared as `Function` so I assumed that I should be returning the `done()` function call. However, [reviewer mentioned](https://github.com/google/blockly-samples/pull/2065#discussion_r1390058446) that returning `done()` is not the correct logic, instead asked to keep the previous logic as is and update the JSDoc comment to a return `{Function | undefined}` instead of `{Function}` which'll cover all possible types of return statements in this function. 

>* Received warning: `Unexpected any. Specify a different type  @typescript-eslint/no-explicit-any` on [L134](https://github.com/google/blockly-samples/blob/master/plugins/block-dynamic-connection/src/dynamic_list_create.ts#L134). Updated changes for a function argument declaration from `state: {[x: string]: any} | string` to `state: {[x: string]: number} | string` to an updated declaration `state: { itemCount?: number, [x: string]: unknown} | string`. My assumption that `state`'s keys will always be number was incorrect. [Reviewer mentioned](https://github.com/google/blockly-samples/pull/2065#discussion_r1390051201) that this argument's keys could be of type number, boolean, string, etc, which I confirmed on their main project Blockly where it is assigned values with all sorts of types. So the ask was to add the key `itemCount` explicitly as an optional key of type number and the rest of the types for keys' values in this object will be changed from `any` to `unknown` to enforce strict type checking. After bit of research I came to know that the reason why `unknown` is strict because it enforces type safety; a developer before assigning the value of a variable declared as `unknown` to another variable, must explicitly check for a valid type and only then they can assign it to another variable. Besides when performing an assignment like this, `this.itemCount = state['itemCount'];` it gave an error since `state`'s `itemCount` key is declared as optional which means the assignment must fallback to a default value when `state` doesn't have a key-value pair of `itemCount`. The reviewer suggested to add nullish coalescing like this `this.itemCount = state['itemCount'] ?? 0;` to account for cases like these.

## Measuring progress from 0.2 release

In the 0.2 release during Hacktoberfest, my contributions primarily revolved around working on small code snippets [_like fixing the order of resolve, reject parameters_](https://dev.to/sdthaker/my-first-ever-hacktoberfest-contribution-14aa), even tasks that did not involve coding [_like fixing transcript for a podcast_](https://dev.to/sdthaker/hacktoberfest-pr-2-5e1e) and much smaller and simpler issues [_like updating a dependency_](https://dev.to/sdthaker/hacktoberfest-pr-3-5a3n) but still added value to the projects. The projects I engaged with [_like Curio_](https://dev.to/sdthaker/hacktoberfest-pr-4-2pbb) were relatively smaller in scale and utilizing a tech stack which I was already familiar with, specifically React.

Now, in the 0.3 release, I pushed myself to the next level. My contributions involved interfacing with a larger and more complex codebase for the project named [blockly-samples](https://github.com/google/blockly-samples). I've been extensively reading and understanding the code written by other developers to add my own contributions. 

Furthermore, this time, I ventured into a project entirely written in TypeScript, a first for me, which is of great significance to me, as it aligns with my aspirations of becoming a web developer. This release has provided me with a valuable opportunity to learn TypeScript, a skill that I believe is essential in the ever-evolving field of web development.

One significant shift in my approach in the 0.3 release was the level of engagement with the project maintainers. I actively interacted with the maintainers, proposing changes, seeking clarification, and requesting their guidance when faced with challenges. This increased communication has not only expanded my knowledge but has also helped in building stronger relationships within the open source community.

Additionally, the 0.3 release was characterized by a significant emphasis on reading and understanding documentation. It involved a specific focus on reading the [eslint-plugin-jsdoc documentation](https://github.com/gajus/eslint-plugin-jsdoc#readme), which was essential for my work on fixing ESLint warnings for JSDoc. Besides, as part of my 0.3 release's contributions, 2 out 4 issues I worked on dealt with ESLint and I'd have to regularly visit its [documentation site](https://eslint.org/docs/latest/) to get better understanding on working with ESLint. Moreover, in my contributions for 0.3 release, I extended my documentation exploration to core web APIs, through the use of well-documented [MDN Web Docs](https://developer.mozilla.org/en-US/docs/Web/JavaScript). This shift reflects my commitment to building a deeper understanding of the tools and technologies I work with, enabling me to contribute more effectively.

A notable aspect of the 0.3 release was the extensive reliance on Document Object Model (DOM) manipulation due to the the nature of the project I chose to work on. To effectively contribute, I often had to refer to the [Blockly's documentation](https://developers.google.com/blockly). I regularly explored [interactive examples for plugins & demos](https://google.github.io/blockly-samples/index.html) provided by the library helping me delve into the functionalities of various plugins to comprehend their role in the project. This experience strengthened my grasp of web development and enriched my understanding of how to create and manipulate elements in the DOM.

In summary, my 0.3 release stands out from the 0.2 release due to its increased complexity in terms of codebase, the introduction of TypeScript, more engagement with maintainers, and a deeper dive into documentation and DOM manipulation. I am excited about the opportunities for further growth and the contributions I can make in future releases.