Customizable cancel/close reason - #14
Open
TiMESPLiNTER wants to merge 9 commits into
Open
Conversation
…ixes - SequentialTaskQueue.cancel(reason?) and close(cancel?, reason?) now accept an optional cancellation reason, mirroring per-task cancel() behavior. Defaults to cancellationTokenReasons.cancel when omitted. - Add GitHub Actions workflow running tests on pull_request (Node 20, npm ci + mocha/ts-node, bypassing the legacy gulp toolchain which is broken on modern Node). - Commit package-lock.json for reproducible installs. - Fix deprecated VS Code tasks.json (isShellCommand) to 2.0.0 schema.
Workspace typescript@2.1.6 crashes parsing modern @types/node syntax (Invalid 'reference' directive syntax), causing the TS language service to cascade into bogus errors like "Type 'typeof internal' has no call signatures" on assert()/setTimeout() calls. Bumping to typescript@5.4 fixes this at the source, keeping import * as assert from "assert" working as-is. Made esModuleInterop explicit (false) in tsconfig.json since that's what makes the namespace-style assert import callable. Also fixes two Promise<void> type errors in examples.ts that were never previously surfaced (old TS couldn't get far enough to check them).
- Add scripts.test running mocha via ts-node/register/transpile-only, bypassing the still-broken legacy gulp test pipeline. - Bump mocha ^3.2.0 -> ^10.0.0 and add ts-node ^10.0.0 as devDependencies so 'npm test' works without ad-hoc npx package fetching. - Simplify .github/workflows/test.yml to 'npm ci && npm test'. - Minor test file cleanup (unused param, added type annotations).
TypeScript 7.0 (now the actual 'latest' on npm) removes esModuleInterop
entirely as a settable option - it's forced on unconditionally. Setting
it to false, which is what made 'import * as assert' callable, is a
hard error on TS7 ('Option esModuleInterop=false has been removed').
Fix: set esModuleInterop: true explicitly (works identically on both
our pinned typescript@5.4/5.9 and any newer TS7 editor/tsdk), and
switch assert/sinon to default imports (import assert from "assert")
in examples.ts to match the pattern already applied to the test file.
Default imports of CJS export= modules stay callable under interop,
regardless of TS version.
Verified clean compile + 39 passing tests on both typescript@5.9.3
(pinned) and typescript@7.0.2.
- timeouts.shift() can return undefined (empty array) - fall back to 0 - var handler: Function declared but only assigned inside a callback; TS can't prove synchronous assignment before use, add definite assignment assertion (var handler!: Function) Verified clean compile on typescript@5.9.3 (pinned) and 7.0.2, 39 tests passing.
gulp@3.9.1's own internal dependency chain (old vinyl-fs/graceful-fs)
crashes on modern Node ('primordials is not defined'), and every gulp
plugin needed a major bump anyway. Rather than maintain a gulp
4/5 rewrite, replaced it entirely with plain npm scripts + native
tools, porting every flow 1:1:
- clean, clean:api -> rm -rf
- build-ts + build-dts -> build (single tsc pass via new
tsconfig.build.json: extends tsconfig.json, only overrides
types/rootDir/outDir/declarationDir, restricted to the lib file)
- test-compile + test-run + test-clean -> test (mocha + ts-node,
transpile-only, no intermediate compiled artifacts to manage)
- test-debug/test-run-debug -> test:debug (--timeout 0)
- doc:readme -> doc:readme (scripts/build-readme.js: same snip-text
snippet extraction from examples.ts + lodash/template rendering of
src/readme.template.md, replacing gulp-template)
- doc:api -> doc:api (typedoc CLI directly)
- doc, prepublish -> doc, prepublishOnly (correct modern npm lifecycle
hook name instead of the no-op-outside-gulp 'prepublish')
Bonus fix while here: doc:api was already broken before this change
(gulp-typedoc pulled in @types/vinyl which its ancient bundled
typedoc@0.5.6/typescript@2.2.2 couldn't parse). Upgraded typedoc
0.5.6 -> 0.28.20 and swapped the unmaintained typedoc-markdown-theme
for typedoc-plugin-markdown (actively maintained, matching peer
version). doc:api now actually works.
Removed devDependencies: del, gulp, gulp-mocha, gulp-rename,
gulp-sourcemaps, gulp-template, gulp-typedoc, gulp-typescript,
run-sequence, vinyl-paths, typedoc-markdown-theme (~500 -> ~150
installed packages).
Regenerated dist/ and readme.md to match current src/examples.
Deleted gulpfile.js and the now-superseded standalone build script
version of tsconfig.build.json's predecessor.
Verified: npm run build, npm test (39 passing), npm run doc, and
npm run prepublishOnly (build+test+doc chained) all pass clean.
rm -rf doesn't work on Windows (cmd.exe/PowerShell). The original gulpfile.js deliberately used the 'del' npm package for this exact reason; the native-tooling migration dropped that guarantee. rimraf is the standard cross-platform equivalent. - clean: rm -rf dist -> rimraf dist - clean:api: rm -rf doc/api -> rimraf doc/api Verified full npm run prepublishOnly chain (build+test+doc) still passes clean.
There was a problem hiding this comment.
Pull request overview
This PR adds support for providing a custom cancellation/closure reason to SequentialTaskQueue.cancel() and SequentialTaskQueue.close(), so cancelled tasks can observe/receive a caller-provided reason. In support of this, it modernizes the project’s build/test/doc tooling (moving off gulp, adding npm scripts, and updating CI) and refreshes generated artifacts/docs.
Changes:
- Extend
SequentialTaskQueue.cancel(reason?)andSequentialTaskQueue.close(cancel?, reason?)to propagate a custom cancellation reason to task promises/tokens. - Modernize tooling: introduce
tsconfig.build.json, npm scripts, and a GitHub Actions test workflow; remove the gulp pipeline. - Update tests/examples/docs and regenerate
dist/outputs accordingly.
Reviewed changes
Copilot reviewed 11 out of 16 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| tsconfig.json | Enables esModuleInterop to support default-import style used in tests/examples. |
| tsconfig.build.json | Adds a dedicated build config targeting dist/lib + dist/types. |
| src/sequential-task-queue.ts | Implements custom cancellation reason plumbing for cancel()/close(). |
| test/sequential-task-queue-spec.ts | Updates imports/types and adds a test for custom cancel reason behavior. |
| scripts/build-readme.js | Adds a node-based README generator replacing the gulp flow. |
| readme.md | Regenerated README content from examples/template. |
| package.json | Replaces gulp tasks with npm scripts and updates dev tooling dependencies. |
| package-lock.json | Adds lockfile v3 and captures updated dependency tree. |
| gulpfile.js | Removes the legacy gulp build/test/doc pipeline. |
| examples/examples.ts | Updates examples (types/imports) used for README snippet generation. |
| dist/types/sequential-task-queue.d.ts | Updates generated type declarations for the new API surface. |
| dist/lib/sequential-task-queue.js | Updates generated JS output for the new API surface/toolchain. |
| dist/lib/sequential-task-queue.js.map | Updates source map for regenerated JS output. |
| .vscode/tasks.json | Updates VS Code build task to run the npm build script. |
| .vscode/settings.json | Updates workspace TS SDK configuration (but needs correction). |
| .github/workflows/test.yml | Adds CI test workflow running npm ci + npm test on Node 20. |
Comments suppressed due to low confidence (6)
readme.md:66
- This code fence is labeled as JavaScript, but the injected snippet contains TypeScript annotations (e.g.,
(token: CancellationToken)). Consider changing the fence language tots(and updatesrc/readme.template.mdaccordingly) so the documentation remains accurate.
```js
readme.md:96
- This code fence is labeled as JavaScript, but the snippet contains TypeScript type annotations (e.g.,
query: string). Consider changing the fence tots(and updatesrc/readme.template.md) so the example matches the declared language.
```js
readme.md:114
- This code fence is labeled as JavaScript, but the snippet uses TypeScript annotations in the callback signature. Consider changing the fence to
ts(and updatingsrc/readme.template.md) to avoid invalid JS examples.
```js
readme.md:126
- This code fence is labeled as JavaScript, but the example uses TypeScript type annotations (e.g.,
data: string). Consider switching the fence tots(and updatingsrc/readme.template.md) so the docs are consistent.
```js
readme.md:161
- This code fence is labeled as JavaScript, but the example contains TypeScript-only syntax (
done: () => void). Consider switching the fence language tots(and updatingsrc/readme.template.md) so the snippet matches the declared language.
```js
src/sequential-task-queue.ts:195
- Minor grammar: “reason of the cancellation” is unidiomatic; “reason for the cancellation” reads more naturally.
* @param {any} reason - The reason of the cancellation, passed to {@link SequentialTaskQueue.cancel} when `cancel` is `true`.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -25,35 +25,35 @@ If the function passed to `push` returns a `Promise`, the queue will wait for it | |||
| Rejected promises don't cause the queue to stop executing tasks, but are reported in the `error` event (see below). | |||
|
|
|||
| ```js | |||
| }, | ||
| "typescript.tsdk": "node_modules/typescript/lib" | ||
|
|
||
| "js/ts.tsdk.path": "node_modules/typescript/lib" |
Comment on lines
+5
to
7
| process.on('unhandledRejection', () => { | ||
| console.log('Suppressed unhandled rejection'); | ||
| }); |
|
|
||
| it("should reject queued tasks with the given reason", () => { | ||
| var queue = new SequentialTaskQueue(); | ||
| var p = queue.push(() => new Promise(resolve => setTimeout(resolve, 200))); |
|
|
||
| /** | ||
| * Cancels the currently running task (if any), and clears the queue. | ||
| * @param {any} reason - The reason of the cancellation, see {@link CancellationToken.reason}. Defaults to {@link cancellationTokenReasons.cancel}. |
Comment on lines
30
to
31
| "@types/mocha": "^2.2.39", | ||
| "@types/node": "^7.0.5", |
Restored the previously hand-added-only 1.2.1 changelog entry into readme.template.md (was dropped when readme.md got regenerated, since it never existed in the template source) and added a 1.3.0 entry for the new SequentialTaskQueue.cancel/close reason parameter. Minor bump since it's a backward-compatible additive change.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR aims to add the possibility to hand a custom reason with
.cancel()and.close(). So each task gets the reason, why the queue has been cancelled or closed.To make it work I had to rebuild a lot of the tooling because the package is very old. I'm not sure if this project is still actively maintained. If yes, it would be nice to bring this feature to master and into a new release.
The actual change can be found here: https://github.com/BalassaMarton/sequential-task-queue/pull/14/changes#diff-a9c5eb0ae0cf5cbd4049404cc13f335640d37a13420093a61c796a987653f5acL173-R204
Additional changes:
npmscripts