Replace native DisposableStack globals with internal shims#41
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis PR introduces an internal disposal framework in src/shims/disposer.ts (symbols, disposal types, InternalDisposableStack, AsyncDisposerStack, createAsyncDisposer, and alias helpers), replaces references to ambient DisposableStack/AsyncDisposableStack with the new AsyncDisposer surface, changes dispose() to return an AsyncDisposer from createAsyncDisposer(), updates executor/modifier/util code to use the shimmed disposers and aliasing (dispose/disposeAsync), adds comprehensive tests (shims and bundle compatibility), and adjusts the build target to browser. Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f8a0ec46e8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| defer(fn: AsyncDisposerFn): void { | ||
| checkCanRegister(this.#disposed, "AsyncDisposableStack") | ||
| this.#stack.push(fn) |
There was a problem hiding this comment.
AsyncDisposerStack.defer currently pushes any value into the cleanup stack without validating that it's callable, so JS consumers (or TS callers using any) can do dispose().defer(123) and only fail later during disposeAsync() with a generic runtime error after work has already executed. Native disposable stacks throw at registration time, and this delayed failure changes control flow in a way that makes cleanup registration bugs much harder to diagnose.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/lib/modifiers/timeout.ts (1)
102-106:⚠️ Potential issue | 🔴 CriticalBuild failure: TypeScript declaration emit rejects computed property with imported symbol.
The pipeline fails with
TS9038: Computed property names in class/object literals must be simple (string/number literals or plain identifiers), not complex expressions.TypeScript's declaration emit cannot handle
[DISPOSE]whenDISPOSEis an imported variable. The pattern of declaring[Symbol.dispose]separately (line 11) and implementing[DISPOSE]()causes the.d.tsgeneration to fail because the computed property name isn't a simple literal.Consider one of these approaches:
- Use a wrapper method pattern - implement a named method and alias it:
dispose(): void { if (this.#timeoutId !== undefined) { clearTimeout(this.#timeoutId) } } get [Symbol.dispose]() { return this.dispose }
Assign the symbol method in the constructor instead of using class method syntax.
Use
isolatedDeclarations: falseif declaration emit flexibility is acceptable.This same issue affects
src/shims/disposer.ts,src/lib/executors/shared.ts,src/lib/executors/base.ts, andsrc/lib/modifiers/signal.tsper the static analysis output.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/modifiers/timeout.ts` around lines 102 - 106, The class currently defines a computed method using the imported symbol DISPOSE ([DISPOSE]()), which breaks TypeScript declaration emit; replace the computed method with a regular named method (e.g., dispose()) that performs the clearTimeout on this.#timeoutId, and then expose the symbol-based accessor by adding a getter get [DISPOSE]() { return this.dispose } (or alternatively assign this[DISPOSE] = this.dispose in the constructor); update the implementation in the timeout modifier (and the other affected modules: shims/disposer, executors/shared, executors/base, modifiers/signal) to use the named method plus symbol getter/assignment so declaration generation succeeds while preserving behavior.
🧹 Nitpick comments (1)
src/__tests__/bundle-compat.test.ts (1)
46-46: Minor: consider usingincludes('from"node:')or a regex for robustness.The check
content.includes('from "node:')assumes single-space formatting. Minified or reformatted bundles might usefrom"node:without a space. A regex like/from\s*["']node:/would be more resilient to formatting variations.♻️ Proposed change
for (const content of contents) { - expect(content.includes('from "node:')).toBe(false) + expect(/from\s*["']node:/.test(content)).toBe(false) expect(content.includes("new DisposableStack")).toBe(false) expect(content.includes("new AsyncDisposableStack")).toBe(false) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/__tests__/bundle-compat.test.ts` at line 46, The test assertion in bundle-compat.test.ts uses content.includes('from "node:') which is fragile to whitespace; update the check around the expect in the test (the expect(...) line in the test block that currently uses content.includes('from "node:')) to use a regex that allows optional whitespace and either quote style, e.g. test the string with /from\s*["']node:/ (or call RegExp.prototype.test) so the expectation becomes expect(/from\s*["']node:/.test(content)).toBe(false) to make the assertion robust to formatting/minification.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.changeset/internal-disposer-shims.md:
- Around line 2-9: The changeset incorrectly labels a breaking API change as a
patch; update the changeset header to a major release and make the intent
explicit: change the version bump for "tryharder" from patch to major, and
update the message to state the public type rename (AsyncDisposableStack →
AsyncDisposer) and removal of runtime globals
(DisposableStack/AsyncDisposableStack) and that consumers must migrate to
dispose() and task-local $disposer; ensure the changeset text clearly documents
the breaking changes so consumers know to update usages of AsyncDisposableStack
and any ambient globals before upgrading.
In `@src/shims/disposer.ts`:
- Around line 95-104: The bug is that use() calls
resolveSyncDisposer()/resolveAsyncDisposer() before ensuring the stack isn't
already disposed, which can invoke user getters or throw instead of failing
fast; fix by checking the stack disposal state via the internal
checkCanRegister() (or by testing this.#disposed) before calling
resolveSyncDisposer/resolveAsyncDisposer, and only call
defer(resolveSyncDisposer(value)) / defer(resolveAsyncDisposer(value)) after
checkCanRegister() succeeds; apply the same ordering change in both the sync use
overload (around function use and resolveSyncDisposer) and the async/other use
overloads (the block at the other affected region).
- Around line 6-9: DISPOSE and ASYNC_DISPOSE are annotated as plain symbol which
causes TS9038 when used as computed property keys; remove the explicit ":
symbol" annotations and instead keep the initializer but assert them to unique
symbol types (e.g., cast the result to unique symbol via "as unknown as unique
symbol") so the exported constants have unique symbol types; update the
declarations for DISPOSE and ASYNC_DISPOSE to use the existing conditional
initializers and append "as unknown as unique symbol" to each expression.
---
Outside diff comments:
In `@src/lib/modifiers/timeout.ts`:
- Around line 102-106: The class currently defines a computed method using the
imported symbol DISPOSE ([DISPOSE]()), which breaks TypeScript declaration emit;
replace the computed method with a regular named method (e.g., dispose()) that
performs the clearTimeout on this.#timeoutId, and then expose the symbol-based
accessor by adding a getter get [DISPOSE]() { return this.dispose } (or
alternatively assign this[DISPOSE] = this.dispose in the constructor); update
the implementation in the timeout modifier (and the other affected modules:
shims/disposer, executors/shared, executors/base, modifiers/signal) to use the
named method plus symbol getter/assignment so declaration generation succeeds
while preserving behavior.
---
Nitpick comments:
In `@src/__tests__/bundle-compat.test.ts`:
- Line 46: The test assertion in bundle-compat.test.ts uses
content.includes('from "node:') which is fragile to whitespace; update the check
around the expect in the test (the expect(...) line in the test block that
currently uses content.includes('from "node:')) to use a regex that allows
optional whitespace and either quote style, e.g. test the string with
/from\s*["']node:/ (or call RegExp.prototype.test) so the expectation becomes
expect(/from\s*["']node:/.test(content)).toBe(false) to make the assertion
robust to formatting/minification.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: add849c0-033c-4be8-a745-4b3baf2c0bed
📒 Files selected for processing (16)
.changeset/internal-disposer-shims.mdREADME.mdbench/suites/orchestration.bench.tsbunup.config.tssrc/__tests__/bundle-compat.test.tssrc/__tests__/dispose.test.tssrc/__tests__/types.test.tssrc/lib/__tests__/coverage-exceptions.test.tssrc/lib/dispose.tssrc/lib/executors/base.tssrc/lib/executors/shared.tssrc/lib/modifiers/signal.tssrc/lib/modifiers/timeout.tssrc/lib/utils.tssrc/shims/disposer.tssrc/types.ts
adelrodriguez
force-pushed
the
03-08-replace_native_disposablestack_globals_with_internal_shims
branch
from
f8a0ec4 to
88e8597
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 88e8597fe5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/lib/executors/base.ts (1)
80-93:⚠️ Potential issue | 🟠 MajorUse explicit try/finally instead of
usingstatement for more direct resource cleanup.The
usingstatement preserves in the output and creates an indirect dependency on Symbol.dispose semantics. While the shim provides Symbol.dispose, explicitly callingdispose()in a try/finally block is more direct and avoids relying on the proposal-level disposal syntax handling through the bundler.♻️ Suggested cleanup path
dispose(): void { if (!this.#timeoutController && !this.#signalController) { return } - using disposer = new InternalDisposableStack() - if (this.#timeoutController) { - disposer.use(this.#timeoutController) - } - - if (this.#signalController) { - disposer.use(this.#signalController) - } + const disposer = new InternalDisposableStack() + try { + if (this.#timeoutController) { + disposer.use(this.#timeoutController) + } + + if (this.#signalController) { + disposer.use(this.#signalController) + } + } finally { + disposer.dispose() + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/executors/base.ts` around lines 80 - 93, The dispose() method uses a `using disposer = new InternalDisposableStack()` which relies on `Symbol.dispose` semantics; change it to explicit try/finally resource cleanup: create `const disposer = new InternalDisposableStack()` (or similar), call `disposer.use(...)` for `this.#timeoutController` and `this.#signalController` as before, then in a finally block call `disposer.dispose()` to ensure deterministic cleanup; update the `dispose()` method (referencing the dispose() function, InternalDisposableStack class, and the private fields `#timeoutController` and `#signalController`) to remove the `using` statement and use the try/finally pattern.
♻️ Duplicate comments (1)
.changeset/internal-disposer-shims.md (1)
2-9:⚠️ Potential issue | 🟠 MajorVersion this as a major release.
Line 2 still marks a source-breaking public API rename as
patch. ReplacingAsyncDisposableStackwithAsyncDisposerwill break existing consumers at compile time, so the changeset needs amajorbump and should call that migration out explicitly.📦 Suggested changeset update
--- -"tryharder": patch +"tryharder": major --- Replace the native `DisposableStack` and `AsyncDisposableStack` runtime dependency with internal private shims. `tryharder` now provides its own cleanup runtime through `dispose()` and task-local `$disposer`, so consumers no longer need native disposable-stack globals or an external polyfill to use the library in unsupported runtimes. -The public cleanup helper type is now `AsyncDisposer` instead of ambient `AsyncDisposableStack`. +This is a breaking change: the public cleanup helper type is now `AsyncDisposer` instead of `AsyncDisposableStack`, and consumers should migrate ambient stack usages to `dispose()` or task-local `$disposer`.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.changeset/internal-disposer-shims.md around lines 2 - 9, The changeset currently labels a breaking API rename as "patch"; update the changeset header to "major" and explicitly state the migration from the ambient AsyncDisposableStack type to the new public AsyncDisposer type so consumers know to update typings; mention that tryharder now embeds internal disposal shims (dispose() runtime and task-local $disposer) replacing the need for native DisposableStack/AsyncDisposableStack or external polyfills and call out that this is a source-breaking rename requiring consumers to replace AsyncDisposableStack references with AsyncDisposer in their code.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@README.md`:
- Around line 514-515: Update the README note to explicitly state that tryharder
supplies the disposal symbols as well as the runtime: clarify that in
environments without native using/DisposableStack support you can call await
disposer.disposeAsync() in a finally block and that tryharder provides both the
cleanup runtime and the necessary symbols (Symbol.dispose and
Symbol.asyncDispose), so native DisposableStack or global symbols are not
required; apply the same clarification to the other occurrence referenced (lines
~680-681) and mention the identifiers `using`, `disposer.disposeAsync()`,
`DisposableStack`, `Symbol.dispose`, and `Symbol.asyncDispose` to make the
change discoverable.
---
Outside diff comments:
In `@src/lib/executors/base.ts`:
- Around line 80-93: The dispose() method uses a `using disposer = new
InternalDisposableStack()` which relies on `Symbol.dispose` semantics; change it
to explicit try/finally resource cleanup: create `const disposer = new
InternalDisposableStack()` (or similar), call `disposer.use(...)` for
`this.#timeoutController` and `this.#signalController` as before, then in a
finally block call `disposer.dispose()` to ensure deterministic cleanup; update
the `dispose()` method (referencing the dispose() function,
InternalDisposableStack class, and the private fields `#timeoutController` and
`#signalController`) to remove the `using` statement and use the try/finally
pattern.
---
Duplicate comments:
In @.changeset/internal-disposer-shims.md:
- Around line 2-9: The changeset currently labels a breaking API rename as
"patch"; update the changeset header to "major" and explicitly state the
migration from the ambient AsyncDisposableStack type to the new public
AsyncDisposer type so consumers know to update typings; mention that tryharder
now embeds internal disposal shims (dispose() runtime and task-local $disposer)
replacing the need for native DisposableStack/AsyncDisposableStack or external
polyfills and call out that this is a source-breaking rename requiring consumers
to replace AsyncDisposableStack references with AsyncDisposer in their code.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 87b19a68-5d39-4ad6-9241-4f6f27d627c3
📒 Files selected for processing (17)
.changeset/internal-disposer-shims.mdREADME.mdbench/suites/orchestration.bench.tsbunup.config.tssrc/__tests__/bundle-compat.test.tssrc/__tests__/dispose.test.tssrc/__tests__/types.test.tssrc/lib/__tests__/coverage-exceptions.test.tssrc/lib/dispose.tssrc/lib/executors/base.tssrc/lib/executors/shared.tssrc/lib/modifiers/signal.tssrc/lib/modifiers/timeout.tssrc/lib/utils.tssrc/shims/__tests__/disposer.test.tssrc/shims/disposer.tssrc/types.ts
✅ Files skipped from review due to trivial changes (1)
- src/shims/tests/disposer.test.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- src/lib/tests/coverage-exceptions.test.ts
- src/lib/utils.ts
- src/tests/bundle-compat.test.ts
- bench/suites/orchestration.bench.ts
- bunup.config.ts
- src/lib/modifiers/signal.ts
adelrodriguez
force-pushed
the
03-08-replace_native_disposablestack_globals_with_internal_shims
branch
from
88e8597 to
f63469f
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/shims/disposer.ts (1)
6-9:⚠️ Potential issue | 🔴 CriticalKeep the exported shim keys as unique symbols.
The
: symbolannotations on Lines 6 and 8 widen both aliases. That breaks the computed members on Lines 15 and 19 and stops these shims from lining up withDisposable/AsyncDisposable. Narrow the initializer totypeof Symbol.dispose/typeof Symbol.asyncDisposeinstead of exporting plainsymbols.Suggested fix
-export const DISPOSE: symbol = - typeof nativeDispose === "symbol" ? nativeDispose : Symbol.for("Symbol.dispose") -export const ASYNC_DISPOSE: symbol = - typeof nativeAsyncDispose === "symbol" ? nativeAsyncDispose : Symbol.for("Symbol.asyncDispose") +export const DISPOSE = + (typeof nativeDispose === "symbol" + ? nativeDispose + : Symbol.for("Symbol.dispose")) as typeof Symbol.dispose +export const ASYNC_DISPOSE = + (typeof nativeAsyncDispose === "symbol" + ? nativeAsyncDispose + : Symbol.for("Symbol.asyncDispose")) as typeof Symbol.asyncDisposeAs per coding guidelines, "Prefer type inference whenever possible in TypeScript."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/shims/disposer.ts` around lines 6 - 9, The exported shim constants DISPOSE and ASYNC_DISPOSE are currently typed as plain symbol which widens them and breaks computed members; change their type annotations to the narrower types matching the native keys (use typeof Symbol.dispose for DISPOSE and typeof Symbol.asyncDispose for ASYNC_DISPOSE) and keep the initializer logic that falls back to Symbol.for when nativeDispose/nativeAsyncDispose aren't present, referencing nativeDispose and nativeAsyncDispose in the same assignments so the shims remain unique and line up with Disposable / AsyncDisposable.
🧹 Nitpick comments (1)
src/shims/disposer.ts (1)
70-103: Let the internal helpers and private methods infer their return types.
resolveSyncDisposer,resolveAsyncDisposer,#disposeAll, and#disposeAllAsyncare all non-public, and inference already gives the right signatures. Dropping those annotations would keep this new file closer to the repo's TypeScript style and reduce manual drift.As per coding guidelines, "Prefer type inference whenever possible in TypeScript" and "Do not add explicit return types unless required by tooling, declaration emit, or a public API contract."
Also applies to: 132-158, 189-217
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/shims/disposer.ts` around lines 70 - 103, The non-public helpers resolveSyncDisposer and resolveAsyncDisposer (and the private methods `#disposeAll` and `#disposeAllAsync`) currently have explicit return type annotations; remove those explicit return types so TypeScript can infer the correct signatures (leave parameter types intact), keeping the file consistent with the repo style and avoiding unnecessary annotations—update the function declarations for resolveSyncDisposer, resolveAsyncDisposer, `#disposeAll` and `#disposeAllAsync` to drop the explicit return type annotations.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/shims/disposer.ts`:
- Around line 50-61: The fallback branch in createSuppressedError currently
reuses the original Error instance (wrapped = error instanceof Error ? error :
new Error(...)), causing a circular reference when you then assign error and
suppressed onto it; change the fallback to always create a fresh Error wrapper
(e.g., new Error(String(errorMessage))) and copy/attach the original error as a
property so the original Error isn't mutated; keep setting name to
"SuppressedError", add suppressed, and retain SUPPRESSED_ERROR_MESSAGE for
context (refer to symbols createSuppressedError, SuppressedError, wrapped,
SUPPRESSED_ERROR_MESSAGE).
---
Duplicate comments:
In `@src/shims/disposer.ts`:
- Around line 6-9: The exported shim constants DISPOSE and ASYNC_DISPOSE are
currently typed as plain symbol which widens them and breaks computed members;
change their type annotations to the narrower types matching the native keys
(use typeof Symbol.dispose for DISPOSE and typeof Symbol.asyncDispose for
ASYNC_DISPOSE) and keep the initializer logic that falls back to Symbol.for when
nativeDispose/nativeAsyncDispose aren't present, referencing nativeDispose and
nativeAsyncDispose in the same assignments so the shims remain unique and line
up with Disposable / AsyncDisposable.
---
Nitpick comments:
In `@src/shims/disposer.ts`:
- Around line 70-103: The non-public helpers resolveSyncDisposer and
resolveAsyncDisposer (and the private methods `#disposeAll` and `#disposeAllAsync`)
currently have explicit return type annotations; remove those explicit return
types so TypeScript can infer the correct signatures (leave parameter types
intact), keeping the file consistent with the repo style and avoiding
unnecessary annotations—update the function declarations for
resolveSyncDisposer, resolveAsyncDisposer, `#disposeAll` and `#disposeAllAsync` to
drop the explicit return type annotations.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1ebc39be-f978-46a7-b492-da0f815a0d3a
📒 Files selected for processing (5)
README.mdsrc/__tests__/bundle-compat.test.tssrc/lib/executors/base.tssrc/shims/__tests__/disposer.test.tssrc/shims/disposer.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- README.md
- src/shims/tests/disposer.test.ts
- src/tests/bundle-compat.test.ts
- src/lib/executors/base.ts
adelrodriguez
force-pushed
the
03-08-replace_native_disposablestack_globals_with_internal_shims
branch
from
f63469f to
9a15a04
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9a15a0493a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
src/shims/disposer.ts (2)
52-63:⚠️ Potential issue | 🟠 MajorAllocate a fresh fallback wrapper for suppressed errors.
When
erroris already anError, Line 57 makeswrapped === error, so Lines 59-63 mutate the original exception and create a self-reference onerror.error. The no-SuppressedErrorpath should always wrap in a newError.🧯 Suggested fix
- const wrapped = error instanceof Error ? error : new Error(String(error)) + const wrapped = new Error(SUPPRESSED_ERROR_MESSAGE) return Object.assign(wrapped, { error, name: "SuppressedError", suppressed, })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/shims/disposer.ts` around lines 52 - 63, The createSuppressedError fallback currently mutates the original Error when error instanceof Error; change it to always create a fresh wrapper Error (e.g., new Error(String(error)) or new Error(error.message) and copy stack if available) so the original exception is not modified, then Object.assign the wrapper with { error, name: "SuppressedError", suppressed } and return the wrapper from createSuppressedError.
6-9:⚠️ Potential issue | 🔴 CriticalType
DISPOSEandASYNC_DISPOSEas unique symbols.
DISPOSEandASYNC_DISPOSEare widened to plainsymbol, so the computed keys at Lines 15 and 19 stop being valid unique-symbol keys for declaration emit. That makes this file a type-check blocker and is also why the casts at Lines 233-234 insrc/shims/__tests__/disposer.test.tsare needed.As per coding guidelines, "Prefer type inference whenever possible in TypeScript".🛠️ Suggested fix
-export const DISPOSE: symbol = - typeof nativeDispose === "symbol" ? nativeDispose : Symbol.for("Symbol.dispose") -export const ASYNC_DISPOSE: symbol = - typeof nativeAsyncDispose === "symbol" ? nativeAsyncDispose : Symbol.for("Symbol.asyncDispose") +export const DISPOSE = + (typeof nativeDispose === "symbol" + ? nativeDispose + : Symbol.for("Symbol.dispose")) as typeof Symbol.dispose +export const ASYNC_DISPOSE = + (typeof nativeAsyncDispose === "symbol" + ? nativeAsyncDispose + : Symbol.for("Symbol.asyncDispose")) as typeof Symbol.asyncDispose🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/shims/disposer.ts` around lines 6 - 9, DISPOSE and ASYNC_DISPOSE are declared as plain symbol which widens their types and prevents them being used as unique-symbol computed keys; change their declarations to use the unique symbol type (e.g., export const DISPOSE: unique symbol = ... and export const ASYNC_DISPOSE: unique symbol = ...) so the compiler treats them as distinct compile-time symbols (this will also remove the need for the casts in the tests referencing those keys); update any related type annotations or exports that assume plain symbol to accept unique symbol.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@README.md`:
- Around line 500-520: The example using "await using disposer = try$.dispose()"
is misleading about disposal timing — it implies disposal at inner block end but
actually disposes when the outer scope exits; update the README demo to use the
explicit try/finally pattern with the AsyncDisposer returned by try$.dispose()
and call disposeAsync() (or cleanup()) in the finally to show immediate cleanup
timing. Reference the AsyncDisposer methods (add, use, cleanup) and the factory
try$.dispose() in the text, and replace or supplement the current "await using"
snippet with a try { ... } finally { await disposer.disposeAsync() } example so
readers clearly see when resources are closed.
In `@src/__tests__/bundle-compat.test.ts`:
- Around line 54-71: The smoke test should force the no-native-symbol branch so
the Reflect.get(Symbol, ...) fallback in src/shims/disposer.ts is exercised;
update the test that writes to smokePath (the writeFile block that imports
entrypoint) to ensure globalThis.Symbol.dispose and
globalThis.Symbol.asyncDispose are undefined prior to doing the dynamic import
(so the import runs with no native Symbol.dispose/asyncDispose), then run the
same run()/all()/flow()/dispose() assertions; locate the writeFile call that
writes the smoke harness (uses variables smokePath and entrypoint) and add the
pre-import globalThis Symbol deletions there.
---
Duplicate comments:
In `@src/shims/disposer.ts`:
- Around line 52-63: The createSuppressedError fallback currently mutates the
original Error when error instanceof Error; change it to always create a fresh
wrapper Error (e.g., new Error(String(error)) or new Error(error.message) and
copy stack if available) so the original exception is not modified, then
Object.assign the wrapper with { error, name: "SuppressedError", suppressed }
and return the wrapper from createSuppressedError.
- Around line 6-9: DISPOSE and ASYNC_DISPOSE are declared as plain symbol which
widens their types and prevents them being used as unique-symbol computed keys;
change their declarations to use the unique symbol type (e.g., export const
DISPOSE: unique symbol = ... and export const ASYNC_DISPOSE: unique symbol =
...) so the compiler treats them as distinct compile-time symbols (this will
also remove the need for the casts in the tests referencing those keys); update
any related type annotations or exports that assume plain symbol to accept
unique symbol.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ba595f96-294d-48d0-9728-d4d2bad47556
📒 Files selected for processing (7)
README.mdsrc/__tests__/bundle-compat.test.tssrc/__tests__/dispose.test.tssrc/__tests__/types.test.tssrc/lib/executors/base.tssrc/shims/__tests__/disposer.test.tssrc/shims/disposer.ts
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/lib/executors/base.ts (1)
80-98: Consider simplifying the dispose method.The current implementation creates an
InternalDisposableStackand wrapsuse()calls in a try-finally, but the finally block always runsdispose(). This is correct, but the try-finally seems unnecessary sinceuse()only throws if the stack is already disposed (which it won't be here) or if the resource isn't disposable (which these controllers are). The pattern could be simplified:♻️ Suggested simplification
dispose(): void { if (!this.#timeoutController && !this.#signalController) { return } const disposer = new InternalDisposableStack() - try { - if (this.#timeoutController) { - disposer.use(this.#timeoutController) - } + if (this.#timeoutController) { + disposer.use(this.#timeoutController) + } - if (this.#signalController) { - disposer.use(this.#signalController) - } - } finally { - disposer.dispose() + if (this.#signalController) { + disposer.use(this.#signalController) } + + disposer.dispose() }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/executors/base.ts` around lines 80 - 98, The dispose() method is overly verbose: it constructs an InternalDisposableStack, calls use() for `#timeoutController` and `#signalController` inside a try-finally, then always calls disposer.dispose(); simplify by removing the try-finally and directly using the stack: create the InternalDisposableStack, call disposer.use(this.#timeoutController) and disposer.use(this.#signalController) only when those fields exist, then call disposer.dispose(); ensure you still early-return when both `#timeoutController` and `#signalController` are falsy and reference the same symbols (dispose(), InternalDisposableStack, `#timeoutController`, `#signalController`, use(), disposer.dispose()) so behavior is preserved while removing the unnecessary try-finally.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/lib/executors/base.ts`:
- Around line 80-98: The dispose() method is overly verbose: it constructs an
InternalDisposableStack, calls use() for `#timeoutController` and
`#signalController` inside a try-finally, then always calls disposer.dispose();
simplify by removing the try-finally and directly using the stack: create the
InternalDisposableStack, call disposer.use(this.#timeoutController) and
disposer.use(this.#signalController) only when those fields exist, then call
disposer.dispose(); ensure you still early-return when both `#timeoutController`
and `#signalController` are falsy and reference the same symbols (dispose(),
InternalDisposableStack, `#timeoutController`, `#signalController`, use(),
disposer.dispose()) so behavior is preserved while removing the unnecessary
try-finally.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 03679cc3-0506-4978-a4e3-a561a927b3ef
📒 Files selected for processing (7)
README.mdsrc/__tests__/bundle-compat.test.tssrc/__tests__/dispose.test.tssrc/__tests__/types.test.tssrc/lib/executors/base.tssrc/shims/__tests__/disposer.test.tssrc/shims/disposer.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- README.md
- Verify fallback/use of runtime disposal symbols - Cover sync/async alias installation and descriptors - Test InternalDisposableStack and async disposer LIFO semantics - Assert shim-only resources are accepted and invalid resources throw
adelrodriguez
force-pushed
the
03-08-replace_native_disposablestack_globals_with_internal_shims
branch
from
9a15a04 to
730bc57
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (2)
README.md (1)
500-520:⚠️ Potential issue | 🟡 MinorShow the non-
await usingcleanup path in the main example.This section still centers the
await usingform, so readers on the runtimes this PR is targeting never see thetry/finally { await disposer.disposeAsync() }fallback that actually works whenusingsyntax is unavailable.📘 Suggested update
-```ts -await using disposer = try$.dispose() - -{ +```ts +const disposer = try$.dispose() + +try { const connection = await db.connect() disposer.add(async () => { await connection.close() }) const user = await connection.users.findById("user_123") +} finally { + await disposer.disposeAsync() }</details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In
@README.mdaround lines 500 - 520, Update the README example to show the
non-await usingcleanup path by replacing theawait using disposer = try$.dispose()block with the explicit try/finally pattern: call try$.dispose()
to get the AsyncDisposer, use disposer.add(...) (or disposer.use(...)) inside
the try block, then call await disposer.disposeAsync() in the finally block so
runtimes withoutusingsyntax see the correct fallback; reference
AsyncDisposer, try$.dispose(), add(), use(), and disposeAsync() in the updated
narrative.</details> </blockquote></details> <details> <summary>src/shims/disposer.ts (1)</summary><blockquote> `6-9`: _⚠️ Potential issue_ | _🔴 Critical_ **Keep the shim disposal keys typed as unique symbols.** The explicit `: symbol` annotations widen both exports away from the well-known symbol types. That makes `[DISPOSE]` / `[ASYNC_DISPOSE]` too loose for computed property declarations here and can break declaration emit for the file. <details> <summary>🔧 Suggested fix</summary> ```diff -export const DISPOSE: symbol = - typeof nativeDispose === "symbol" ? nativeDispose : Symbol.for("Symbol.dispose") -export const ASYNC_DISPOSE: symbol = - typeof nativeAsyncDispose === "symbol" ? nativeAsyncDispose : Symbol.for("Symbol.asyncDispose") +export const DISPOSE = + (typeof nativeDispose === "symbol" + ? nativeDispose + : Symbol.for("Symbol.dispose")) as typeof Symbol.dispose +export const ASYNC_DISPOSE = + (typeof nativeAsyncDispose === "symbol" + ? nativeAsyncDispose + : Symbol.for("Symbol.asyncDispose")) as typeof Symbol.asyncDisposeAs per coding guidelines, "Prefer type inference whenever possible in TypeScript" and "Do not use explicit return types when inference already produces the correct result."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/shims/disposer.ts` around lines 6 - 9, Remove the explicit ": symbol" annotations on the DISPOSE and ASYNC_DISPOSE exports so TypeScript infers the exact unique/well-known symbol types; locate the const declarations for DISPOSE and ASYNC_DISPOSE (which use nativeDispose and nativeAsyncDispose in the ternary expressions) and simply drop the type annotations, leaving the ternary expressions intact so the variables keep their precise inferred symbol types instead of being widened.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@README.md`:
- Around line 500-520: Update the README example to show the non-`await using`
cleanup path by replacing the `await using disposer = try$.dispose()` block with
the explicit try/finally pattern: call try$.dispose() to get the AsyncDisposer,
use disposer.add(...) (or disposer.use(...)) inside the try block, then call
await disposer.disposeAsync() in the finally block so runtimes without `using`
syntax see the correct fallback; reference AsyncDisposer, try$.dispose(), add(),
use(), and disposeAsync() in the updated narrative.
In `@src/shims/disposer.ts`:
- Around line 6-9: Remove the explicit ": symbol" annotations on the DISPOSE and
ASYNC_DISPOSE exports so TypeScript infers the exact unique/well-known symbol
types; locate the const declarations for DISPOSE and ASYNC_DISPOSE (which use
nativeDispose and nativeAsyncDispose in the ternary expressions) and simply drop
the type annotations, leaving the ternary expressions intact so the variables
keep their precise inferred symbol types instead of being widened.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3734158b-605c-44ad-ad6d-ba9ce0e128ee
📒 Files selected for processing (8)
README.mdsrc/__tests__/bundle-compat.test.tssrc/__tests__/dispose.test.tssrc/__tests__/types.test.tssrc/lib/__tests__/coverage-exceptions.test.tssrc/lib/executors/base.tssrc/shims/__tests__/disposer.test.tssrc/shims/disposer.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/tests/bundle-compat.test.ts
adelrodriguez
deleted the
03-08-replace_native_disposablestack_globals_with_internal_shims
branch
* Replace native `DisposableStack` globals with internal shims * Add disposer shim tests for symbol aliases and disposal behavior - Verify fallback/use of runtime disposal symbols - Cover sync/async alias installation and descriptors - Test InternalDisposableStack and async disposer LIFO semantics - Assert shim-only resources are accepted and invalid resources throw
* Replace native `DisposableStack` globals with internal shims * Add disposer shim tests for symbol aliases and disposal behavior - Verify fallback/use of runtime disposal symbols - Cover sync/async alias installation and descriptors - Test InternalDisposableStack and async disposer LIFO semantics - Assert shim-only resources are accepted and invalid resources throw
1 participant
This pull request replaces the native
DisposableStackandAsyncDisposableStackruntime dependency with internal private shims, eliminating the need for native disposable-stack globals or external polyfills in unsupported runtimes.Closes #36
Key Changes
AsyncDisposerandInternalDisposableStackimplementations insrc/shims/disposer.tsthat provide the same functionality as native disposable stacksdispose()function now returnsAsyncDisposerinstead ofAsyncDisposableStack, with the new type exported fromtryharder/typesawait disposer.disposeAsync()infinallyblocks whenusingsyntax is unavailable, and removed the previous runtime limitation warningsThe library now provides its own cleanup runtime through
dispose()and task-local$disposer, making it compatible with Firefox, Safari, and other runtimes that don't yet support native disposable stacks.