chore: enable strict mode for workspace-minimap

[Namatuzio]

The basics

• I validated my changes

The details

Resolves

Fixes #2031

Proposed Changes

• Updates the workspace-minimap plugin's type checks in accordance with TypeScript's strict-mode
• Updated the following files:
  • plugins\workspace-minimap\src\minimap.ts
  • plugins\workspace-minimap\src\focus_region.ts
  • plugins\workspace-minimap\test\index.ts
  • plugins\workspace-minimap\test\minimap_tests.mocha.js

Reason for Changes

Strict mode is a great way to ensure strong code consistency and maintain high code standards. It allows for fewer faults within code to be possible by patching potential oversights.

Test Coverage

npm run test results:

Documentation

Additional Information

There are 2 warnings when running npm run test from a change I added. For some reason when I ran the tests, the tests wouldn't have access to the document object part of the DOM, so I added the following to the top of the tester file which seemed to have fixed it:

const jsdom = require('jsdom');
const { JSDOM } = jsdom;

const { document } = (new JSDOM('')).window;
global.document = document;

[google-cla]

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

[BeksOmega]

Thank you for your work on this! This looks like a great start :D

[Namatuzio]

Amazing feedback, thank you! I'll get right on the changes needed to be made!

[Namatuzio]

I updated the PR as per the requested changes! As always, let me know if anything else needs to be further updated, thanks!

[Namatuzio]

@BeksOmega I added the requested changes!

• primaryInjectParentDiv's initialization was added to the top of the init() method along with the null check
• minimapWorkspace and focusRegion were made nullable and null checks were added where necessary
• Formatting updates and newlines where needed

[BeksOmega]

There are 2 warnings when running npm run test from a change I added. For some reason when I ran the tests, the tests wouldn't have access to the document object part of the DOM, so I added the following to the top of the tester file which seemed to have fixed it:

When I tested removing the JSDOM code locally, I didn't receive any errors :/ Could you post back with the errors you're receiving? Hopefully with a bit more info we can figure out what's up!

[Namatuzio]

There are 2 warnings when running npm run test from a change I added. For some reason when I ran the tests, the tests wouldn't have access to the document object part of the DOM, so I added the following to the top of the tester file which seemed to have fixed it:

When I tested removing the JSDOM code locally, I didn't receive any errors :/ Could you post back with the errors you're receiving? Hopefully with a bit more info we can figure out what's up!

Of course! So funnily enough, yesterday when I tested it, it was working perfectly fine without the code, which confused me.

I decided to make another local branch on my machine to test it out. I added some of the changes from the PR and luckily I was able to reproduce it:

  1) Positioning the minimap in the primary workspace
       LTR Vertical Start:
     ReferenceError: document is not defined
      at measureFontMetrics$$module$build$src$core$utils$dom (webpack-internal:///./node_modules/blockly/blockly_compressed.js:98:79)
      at ConstantProvider$$module$build$src$core$renderers$geras$constants.setFontConstants_ (webpack-internal:///./node_modules/blockly/blockly_compressed.js:1001:478)
      at ConstantProvider$$module$build$src$core$renderers$geras$constants.setDynamicProperties_ (webpack-internal:///./node_modules/blockly/blockly_compressed.js:1001:93)
      at ConstantProvider$$module$build$src$core$renderers$geras$constants.setTheme (webpack-internal:///./node_modules/blockly/blockly_compressed.js:1001:38)
      at Renderer$$module$build$src$core$renderers$geras$renderer.init (webpack-internal:///./node_modules/blockly/blockly_compressed.js:1181:684)
      at Renderer$$module$build$src$core$renderers$geras$renderer.init (webpack-internal:///./node_modules/blockly/blockly_compressed.js:1511:379)
      at init$$module$build$src$core$renderers$common$block_rendering (webpack-internal:///./node_modules/blockly/blockly_compressed.js:334:181)
      at new WorkspaceSvg$$module$build$src$core$workspace_svg (webpack-internal:///./node_modules/blockly/blockly_compressed.js:1367:131)
      at new Minimap (webpack-internal:///./src/minimap.ts:45:33)
      at new PositionedMinimap (webpack-internal:///./src/positioned_minimap.ts:30:9)

I took a look at the errors that were being thrown and realized it was stemming from minimap.ts so I went to the line that was causing it and found the culprit!

  protected primaryWorkspace: Blockly.WorkspaceSvg;
  protected minimapWorkspace: Blockly.WorkspaceSvg;
  protected focusRegion: FocusRegion;
  protected onMouseMoveWrapper: Blockly.browserEvents.Data | null = null;
  protected onMouseDownWrapper: Blockly.browserEvents.Data | null = null;
  protected onMouseUpWrapper: Blockly.browserEvents.Data | null = null;
  protected minimapWrapper: HTMLDivElement;

  /**
   * Constructor for a minimap.
   *
   * @param workspace The workspace to mirror.
   */
  constructor(workspace: Blockly.WorkspaceSvg) {
    this.primaryWorkspace = workspace;
    this.minimapWorkspace = new Blockly.WorkspaceSvg(new Blockly.Options({}));
    this.focusRegion = new FocusRegion(this.primaryWorkspace, this.minimapWorkspace);
    this.minimapWrapper = document.createElement('div');
  }

It was all caused by the old way I handled the constructor! This makes sense as to why in my initial PR, I noted the issue.

Of course with the new changes to minimap.ts this error does not exist!

  protected primaryWorkspace: Blockly.WorkspaceSvg;
  protected minimapWorkspace: Blockly.WorkspaceSvg | null = null;
  protected focusRegion: FocusRegion | null = null;
  protected onMouseMoveWrapper: Blockly.browserEvents.Data | null = null;
  protected onMouseDownWrapper: Blockly.browserEvents.Data | null = null;
  protected onMouseUpWrapper: Blockly.browserEvents.Data | null = null;
  protected minimapWrapper: HTMLDivElement | null = null;

  /**
   * Constructor for a minimap.
   *
   * @param workspace The workspace to mirror.
   */
  constructor(workspace: Blockly.WorkspaceSvg) {
    this.primaryWorkspace = workspace;
  }

I'll make some changes to the PR once more, removing the tester file code that I added so we can get this merged. Thanks again @BeksOmega for the opportunity to work on this!

[BeksOmega]

Sweet this looks great @Namatuzio Thank you for your hard work on this :D You just need to run npm run format and commit the changes, then this should be good to go!

[Namatuzio]

Sweet this looks great @Namatuzio Thank you for your hard work on this :D You just need to run npm run format and commit the changes, then this should be good to go!

I formatted the specific files that were causing errors in the build so everything should work just fine now. Thanks a ton!