Global throttling

[cwillisf]

Resolves

• Resolves CE-132 Loading a project with many assets can fail scratch-gui#7111 and POD-238

Proposed Changes

Main change: run scratchFetch through a throttling queue

Supporting changes:

• Convert remaining JavaScript files in src/ to TypeScript (namely scratchFetch and the FetchWorkerTool worker)
• Removed cross-fetch dependency
• Lots of work on tests

WARNING: This build will fail until the task-herder dependency here is updated to a version that includes the work in scratchfoundation/scratch-editor#393.

Reason for Changes

Providing a global throttling mechanism helps prevent technical issues like scratchfoundation/scratch-gui#7111 and non-technical issues like overuse of our APIs.

The cross-fetch library was only necessary when fetch was not reliably available. It's now baseline widely available, so we no longer need to worry about that.

Future Work

Consider using this mechanism for cloud variables (to prevent the current disconnect/reconnect cycle) and Bluetooth extensions (to avoid bad behavior in hardware devices and OS Bluetooth stacks).

Test Coverage

The test work here covers the throttling changes as well as the TypeScript changes. I wanted to be certain that converting to TypeScript didn't change the interface even for JS callers. The details of mocking fetch also changed with the removal of cross-fetch. Finally, since scratchFetch inspects the requested URL to determine which queue to use, I had to adjust the tests to use real URLs.

[Copilot]

Pull request overview

This PR implements a global throttling mechanism for network requests by routing all scratchFetch calls through a queue manager system. It converts the remaining JavaScript files in src/ (specifically scratchFetch and FetchWorkerTool.worker) to TypeScript and removes the cross-fetch dependency in favor of the native fetch API which is now baseline available in target environments.

Key Changes:

• Introduces HostQueues.ts with a centralized QueueManager that throttles requests per hostname with configurable limits
• Converts scratchFetch from CommonJS to ES6 TypeScript with new ScratchFetchOptions type for queue configuration
• Converts FetchWorkerTool.worker.js to TypeScript with async/await patterns and proper typing

Reviewed changes

Copilot reviewed 16 out of 19 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
src/HostQueues.ts	New file defining queue options for assets and creating the central host queue manager
src/scratchFetch.ts	Converted to TypeScript with ES6 exports; integrated throttling via queue manager
src/FetchWorkerTool.worker.ts	Converted to TypeScript with async/await and queue integration
src/FetchTool.ts	Updated to pass AssetQueueOptions to scratchFetch calls
src/ScratchStorage.ts	Changed import from default to namespace import for scratchFetch
src/types.d.ts	Simplified module declarations to use wildcard pattern
test/fixtures/mockFetch.js	Refactored to mock global fetch instead of cross-fetch
test/fixtures/known-assets.js	Added JSDoc type definitions for better type documentation
test/unit/*.test.js	Updated to use new mock setup and real URLs instead of mock paths
test/integration/download-known-assets.test.js	Updated URLs and comments for new fetch mock
test/build/*.test.js	New tests verifying build output and public API
tsconfig.json	Removed types configuration
tsconfig.test.json	Replaced module: "CommonJS" with isolatedModules: true
package.json	Removed cross-fetch dependency; added test:build script
package-lock.json	Removed cross-fetch and marked node-fetch as dev dependency
.github/workflows/ci-cd.yml	Split tests into pre-build and post-build phases

Comments suppressed due to low confidence (2)

src/scratchFetch.ts:1

• The import path '../../scratch-editor/packages/task-herder/dist/TaskQueue' uses a relative path that goes outside the project (../../scratch-editor). This should use a package import like '@scratch/task-herder' instead, similar to how it's imported in HostQueues.ts line 1. This relative path will break if the repository structure changes and is not a standard way to import from dependencies.
  test/fixtures/mockFetch.js:6
• The example in the documentation comment refers to a path '../mocks/fetch' but the actual file is located at '../fixtures/mockFetch.js'. This should be updated to require('../fixtures/mockFetch').fetch or similar to match the actual file location and module structure.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Pull request overview

Copilot reviewed 16 out of 19 changed files in this pull request and generated 2 comments.

Comments suppressed due to low confidence (3)

src/scratchFetch.ts:112

• There's a potential issue with option handling. When queueName is not provided:

1. Line 108 creates a new Request with requestOptions: resource = new Request(resource, requestOptions)
2. Line 112 then passes requestOptions again to fetch: fetch(resource, requestOptions)

This means requestOptions are being applied twice - once when creating the Request, and again when calling fetch. While the Fetch API allows this, it's redundant and could lead to unexpected behavior if options conflict.

Consider one of these approaches:

• Option A: Only pass requestOptions to the Request constructor, then call fetch(resource) without options
• Option B: Track whether resource was normalized and conditionally pass options: fetch(resource, queueName ? requestOptions : undefined)

Option A is cleaner:

if (!queueName) {
    resource = new Request(resource, requestOptions);
    queueName = new URL(resource.url).hostname;
    requestOptions = undefined; // options already applied to Request
}
const queue = hostQueueManager.getOrCreate(queueName, scratchOptions?.queueOptions);
return queue.do(() => fetch(resource, requestOptions));

src/scratchFetch.ts:1

• The import path uses a relative path ../../scratch-editor/packages/task-herder/dist/TaskQueue which appears to reference a local monorepo structure. This is inconsistent with line 1 of src/HostQueues.ts which uses the npm package name @scratch/task-herder.

Consider using the npm package name consistently: import {type QueueOptions} from '@scratch/task-herder';
test/fixtures/mockFetch.js:6

• The example in the comment references '../mocks/fetch' but the actual file is located at '../fixtures/mockFetch'. Update the comment to reflect the correct path:

// global.fetch = require('../fixtures/mockFetch').fetch;

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

[nitpick] The check expect(storage.scratchFetch).toBeDefined() at line 20 is redundant since it's already checked in the test at line 15. Consider removing this line as it doesn't add value to the test.

Suggested change

expect(storage.scratchFetch).toBeDefined();

[Copilot]

The test is calling setMetadata with arbitrary string values ('foo', 'FOO') instead of RequestMetadata enum values. The TypeScript signature in src/scratchFetch.ts expects the first parameter to be of type RequestMetadata, which only includes 'X-Project-ID' and 'X-Run-ID'.

If the function should support arbitrary header names for testing purposes, consider updating the TypeScript signature to accept RequestMetadata | string instead of just RequestMetadata. Otherwise, update the test to use valid enum values.