Skip to content

Conversation

cwillisf
Copy link
Contributor

@cwillisf cwillisf commented Sep 24, 2025

Proposed Changes

  • Update several dependencies:
    • eslint: v8 to v9
    • eslint-config-scratch: v9 to v12
    • jest and ts-jest: v21 to v25
      • updated test snapshots (new jest doesn't record undefined properties, etc.)
    • various support / plugin updates as well
  • Remove legacy eslint configuration files and create new, flat ones
  • Enable TypeScript & type-aware linting in scratch-gui
  • Fix tests broken by all these updates

Reason for Changes

Test Coverage

Covered by existing tests, which have been updated to accommodate these changes.

@cwillisf cwillisf requested a review from a team as a code owner September 24, 2025 15:47
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR updates ESLint from version 8 to 9 and eslint-config-scratch from version 9 to 12, along with associated testing framework upgrades (Jest v21 to v25, ts-jest updates). The main change involves migrating from legacy ESLint configuration files to the new flat configuration format, while maintaining existing linting rules and behavior.

  • Migration from legacy .eslintrc.js files to new eslint.config.mjs flat configuration format
  • Update of core dependencies (ESLint v9, eslint-config-scratch v12, Jest v25)
  • Replacement of process.nextTick() with setTimeout() in tests due to Jest environment changes

Reviewed Changes

Copilot reviewed 79 out of 81 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
packages/scratch-vm/eslint.config.mjs New flat ESLint config replacing legacy .eslintrc.js files
packages/scratch-svg-renderer/eslint.config.mjs New flat ESLint config with browser/node environment settings
packages/scratch-render/eslint.config.mjs New flat ESLint config with playground-specific rule overrides
packages/scratch-gui/eslint.config.mjs New comprehensive flat ESLint config with TypeScript support
packages/scratch-gui/test/unit/util/detect-locale.test.js Refactored test to use Jest spies instead of Object.defineProperty
packages/scratch-gui/test/unit/util/vm-manager-hoc.test.jsx Updated async test patterns for Jest v25 compatibility
packages/scratch-gui/src/lib/legacy-storage.ts Added null check for projectHost property

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

Comment on lines 103 to 105
* Set a timeout for a function to be called after a delay.
* @param {Function} fn The function to call after the delay.
* @param {number} time The delay time in milliseconds.
Copy link

Copilot AI Sep 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nitpick] The JSDoc comment doesn't add value since it only restates what the method name and parameters already make clear. Consider removing this documentation or enhancing it to explain the specific context of why this timeout is being set (reconnection logic).

Suggested change
* Set a timeout for a function to be called after a delay.
* @param {Function} fn The function to call after the delay.
* @param {number} time The delay time in milliseconds.
* Schedule a reconnection attempt to the cloud data server after a websocket disconnect.
* This method manages the delay (with exponential backoff and jitter) before trying to reconnect,
* helping to avoid overwhelming the server with rapid reconnection attempts.
* @param {Function} fn The function to call after the delay (typically to reopen the connection).
* @param {number} time The delay time in milliseconds before attempting to reconnect.

Copilot uses AI. Check for mistakes.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added a jsdoc comment specifically to make TS linting happy about the arg types

Copy link

github-actions bot commented Sep 24, 2025

Test report for scratch-svg-renderer

  1 files  ±0   60 suites  ±0   0s ⏱️ ±0s
124 tests ±0  124 ✅ ±0  0 💤 ±0  0 ❌ ±0 
276 runs  ±0  275 ✅ ±0  1 💤 ±0  0 ❌ ±0 

Results for commit e333f9c. ± Comparison against base commit 893c679.

♻️ This comment has been updated with latest results.

Copy link

github-actions bot commented Sep 24, 2025

Test report for scratch-render

  1 files  ±0   55 suites  ±0   2s ⏱️ ±0s
209 tests ±0  209 ✅ ±0  0 💤 ±0  0 ❌ ±0 
279 runs  ±0  279 ✅ ±0  0 💤 ±0  0 ❌ ±0 

Results for commit e333f9c. ± Comparison against base commit 893c679.

♻️ This comment has been updated with latest results.

Copy link

github-actions bot commented Sep 24, 2025

Test report for scratch-vm

    1 files  ±0    770 suites  ±0   1m 5s ⏱️ ±0s
1 686 tests ±0  1 686 ✅ ±0   0 💤 ±0  0 ❌ ±0 
4 891 runs  ±0  4 861 ✅ ±0  30 💤 ±0  0 ❌ ±0 

Results for commit e333f9c. ± Comparison against base commit 893c679.

♻️ This comment has been updated with latest results.

Copy link

github-actions bot commented Sep 24, 2025

Test report for scratch-gui

  2 files  ±0   62 suites  ±0   9m 51s ⏱️ + 1m 3s
398 tests ±0  390 ✅ ±0  8 💤 ±0  0 ❌ ±0 
416 runs  ±0  408 ✅ ±0  8 💤 ±0  0 ❌ ±0 

Results for commit e333f9c. ± Comparison against base commit 893c679.

♻️ This comment has been updated with latest results.

Comment on lines +72 to +74
if (!this.projectHost) {
return Promise.reject(new Error('Project host not set'));
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Without this, TS said that passing this.projectHost to saveProjectToServer was invalid because it might be undefined.

storage.load = originalLoad;
// nextTick needed since storage.load is async, and onFetchedProject is called in its then()
process.nextTick(
setTimeout(
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For some reason, process.nextTick wasn't delaying long enough. Waiting for 1ms seems to work. It wouldn't surprise me if this turns out to be flaky, though. These tests should ideally be event-driven (watch for a "load finished" event or something), but that'd be a larger change than I wanted to integrate into this PR.

Comment on lines -168 to +172
"ts-jest": "21.2.4",
"ts-jest": "^25.5.1",
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was surprised to find that I had to update jest and ts-jest to get this all to work. I'm not sure exactly what the interaction was, but once I updated all the eslint-related things, jest started to fail with a parse error.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I figured it out, at least partially: [email protected] requires typescript@^2.4.1, but @typescript-eslint/eslint-plugin requires typescript>=4.8.4 <6.0.0. Npm "helpfully" resolves this by (among other things) providing a newer jest-cli to scratch-gui than the [email protected] that scratch-gui/package.json asks for. Bringing in a newer version of Jest introduces several new requirements that seem to be met by properly updating jest and ts-jest.

There are still a few holes in my understanding here, like why npm makes available a version of jest that doesn't match package.json instead of throwing an error, but I'm at least closer to understanding it 😅

Comment on lines 357 to 365
/**
* States the video sensing activity can be set to.
* @readonly
* @enum {string}
*/
static get VideoState () {
return VideoState;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was unused and causes a namespace collision for TS

@KManolov3
Copy link
Contributor

KManolov3 commented Sep 29, 2025

@cwillisf It's likely going to not be pretty, but should we try pointing this to https://github.com/scratchfoundation/scratch-editor/tree/UEPR-282-face-sensing, so that we are ready when it ends up being merged into develop in the next week or so?

@cwillisf
Copy link
Contributor Author

@cwillisf It's likely going to not be pretty, but should we try pointing this to UEPR-282-face-sensing, so that we are ready when it ends up being merged into develop in the next week or so?

Probably a good idea... I'll see what it looks like.

"\\.(css|less)$": "<rootDir>/test/__mocks__/styleMock.js",
"editor-msgs(\\.js)?$": "<rootDir>/test/__mocks__/editor-msgs-mock.js"
"editor-msgs(\\.js)?$": "<rootDir>/test/__mocks__/editor-msgs-mock.js",
"^@scratch/scratch-svg-renderer$": "<rootDir>/../scratch-svg-renderer/src/"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just curious - why did we end up needing this for svg-renderer, but not for e.g. -vm

Copy link
Contributor

@KManolov3 KManolov3 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's quite a few changes! Let's wait for the face sensing release before merging this in develop, just to be sure.

@cwillisf cwillisf force-pushed the eslint-9-scratch-12 branch from 43c2233 to e56dc93 Compare October 16, 2025 16:56
@cwillisf
Copy link
Contributor Author

I just rebuilt this on top of the React 18 and Face Sensing changes. I also ran eslint --fix since these changes caused so many new lint warnings. Looks like Renovate did its thing while I had my back turned, but it should only take a little bit more merging before this is ready to go...

@cwillisf cwillisf added this pull request to the merge queue Oct 16, 2025
Merged via the queue into develop with commit 6662051 Oct 16, 2025
19 of 20 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Oct 16, 2025
@cwillisf cwillisf deleted the eslint-9-scratch-12 branch October 16, 2025 18:11
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants