Update to eslint v9 and eslint-config-scratch v12 #327
Conversation
Also remove eslint@^8 plugins/helpers. This commit by itself will break linting! Followup needed: - Upgrade to eslint@^9-compatible version of eslint-config-scratch - Migrate config files - Probably fix some legacy linting rules
There was a problem hiding this comment.
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.jsfiles to neweslint.config.mjsflat configuration format - Update of core dependencies (ESLint v9, eslint-config-scratch v12, Jest v25)
- Replacement of
process.nextTick()withsetTimeout()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.
| * 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. |
There was a problem hiding this comment.
[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).
| * 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.
There was a problem hiding this comment.
I added a jsdoc comment specifically to make TS linting happy about the arg types
| if (!this.projectHost) { | ||
| return Promise.reject(new Error('Project host not set')); | ||
| } |
There was a problem hiding this comment.
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( |
There was a problem hiding this comment.
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.
| "ts-jest": "21.2.4", | ||
| "ts-jest": "^25.5.1", |
There was a problem hiding this comment.
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.
| /** | ||
| * States the video sensing activity can be set to. | ||
| * @readonly | ||
| * @enum {string} | ||
| */ | ||
| static get VideoState () { | ||
| return VideoState; | ||
| } | ||
|
|
There was a problem hiding this comment.
This was unused and causes a namespace collision for TS
|
@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? |
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/" |
There was a problem hiding this comment.
Just curious - why did we end up needing this for svg-renderer, but not for e.g. -vm
Proposed Changes
eslint: v8 to v9eslint-config-scratch: v9 to v12jestandts-jest: v21 to v25jestdoesn't recordundefinedproperties, etc.)scratch-guiReason for Changes
Test Coverage
Covered by existing tests, which have been updated to accommodate these changes.