Delving into open source territory with Google: PR #1

Project repo:

blockly-samples by Google Open Source.

Project description:

Blockly by Google Open Source is a JavaScript library for building visual programming editor, which looks something like this,

The project I worked on is called blockly-samples which is a supplementary project to the Blockly project. This project holds code for 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 which are self-contained sample projects demonstrating techniques on how to use Blockly's features and extend the Blockly library and Codelabs which are interactive tutorials demonstrating how to use and customize Blockly.

Issue

1978

PR

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 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,

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,

Example #2

Another warning I received was on line 134 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,

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,

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

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,

Example: A couple of hard fixes

Example #1

A warning I was trying to fix was on line 221 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 class and I had to add an explanation about the purpose of the function and its return value. Click here to interact with the FieldBitmap plugin. Visually, it looks like this,

The code before the fix is below,

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 class file where the variable is declared on 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. 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 implemented SVGElement which in turn implemented Element which is where I finally found the getBoundingClientRect() implementation. The return value of this function is an SVG's group element's (an instance of 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 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,

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

for this code,

located in the file /blockly-samples/plugins/migration/bin/fix-imports.js part of keyboard-navigation plugin. Its interactive demo can be found here. The purpose of this plugin is to provide keyboard navigation for better accessibility through a keyboard cursor. You can find its documentation site here 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 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,

/**
* @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. 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 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. Removed explicit cast of a function from createWorkspace as any to

    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 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. 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 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. 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 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, even tasks that did not involve coding like fixing transcript for a podcast and much smaller and simpler issues like updating a dependency but still added value to the projects. The projects I engaged with like Curio 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. 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, 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 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. 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. I regularly explored interactive examples for plugins & demos 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.