migrate to typescript - #17
Draft
sderrow wants to merge 12 commits into
Draft
Conversation
Owner
Author
|
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
This stack of pull requests is managed by Graphite. Learn more about stacking. |
Pure rename, no content changes. Code changes follow in the next commit.
This was referenced Aug 30, 2026
- Convert all src modules from CJS to ESM with strict types (shared shapes in src/types.ts, structural peer-client types in src/cluster/redis-types.ts) - ESM-cycle fix: Group <-> Bottleneck now use a lazy static accessor for Bottleneck.Group (a static field initializer hits the TDZ when Group.ts is imported first under ESM) - package.json: type: module; dist emitted as CJS via .cjs (dist/index.cjs, dist/light.cjs) so require() consumers keep working; tsdown entries point at src/index.ts - Tests: convert require() of src/bridge/helpers to ESM imports, bridge becomes test/bottleneck.mjs, keep-alive fixtures run through tsx so src loads with extensionless specifiers - tsconfig includes src (strict) and scripts Known issue: cluster-coordination 'pass messages after Group re-instantiations' (node-redis project) intermittently times out due to a node-redis v6 subscribe/unsubscribe race; under investigation separately. Modernize the light build: UMD -> ESM - dist/light.js is now a pure ESM build for <script type="module"> and bundlers; the CJS-flavored UMD wrapper and its .cjs naming (awkward under type: module) are gone - exports["./light"] points at dist/light.js; README updated Build the main library in dual ESM/CJS formats - tsdown lib config: format: [esm, cjs] -> dist/index.mjs + dist/index.cjs; exports map gains import/require conditions (types still pinned to bottleneck.d.ts until generated dts lands) - README build docs updated Build the main library in dual ESM/CJS formats - tsdown lib config: format: [esm, cjs] -> dist/index.mjs + dist/index.cjs; exports map gains import/require conditions (types still pinned to bottleneck.d.ts until generated dts lands) - README build docs updated browser for light
- dts: true on both tsdown builds: dist/index.d.mts/.d.cts and dist/light.d.ts are now generated from the typed source - exports map: per-condition types (import -> index.d.mts, require -> index.d.cts); ./light -> light.d.ts - Delete the hand-written bottleneck.d.ts (626 lines) and the consumer test.ts that exercised it; the published types are now generated from the same source the runtime tests cover - Tighten public types to (and beyond) the old contract: ConstructorOptions/JobOptions/StopOptions/BatcherOptions with docs, Strategy as literal union (1|2|3|4), typed event maps (BottleneckEvents/GroupEvents/BatcherEvents), schedule()/wrap() overloads that unwrap async task results (Awaited<R>) and accept options+args, honest return types (running/done -> Promise<number>, currentReservoir/incrementReservoir -> Promise<number | null>) - Public types re-exported from the package root; types.test.ts asserts the contract with expectTypeOf
- Pure rename of all test/**/*.js -> .ts (plus bottleneck.mjs -> bottleneck.ts). Pre-commit lint is skipped for this commit: oxlint's vitest rules apply to .ts files, so the hook cannot pass on a content-free rename; lint/type fixes follow in the next commit. - Also removes the hand-written bottleneck.d.ts and its consumer test.ts from the repo root (superseded by generated dts and test/types.test.ts) — follow-up missed by the dts commit's pathspec.
- Foundation: test/bottleneck.ts bridge gets an explicit class type (typeof BottleneckBase); helpers typed (limiter, job-tasks, wait-for-state, redis-client-options, leakage); test-api.ts gains a typed fixtures contract (JobHarness, MakeLimiter/MakeGroup/ MakeConnection, Track) plus vitest Assertion declaration-merging for the custom toHaveCallOrder/toHaveFinalCallAt/toHaveCallAt matchers - All test files typed: vi.fn type parameters, callback params, non-null assertions under noUncheckedIndexedAccess, casts for library internals (_store/connection) and stub objects; no runtime test semantics changed - vitest project include/exclude globs updated .js -> .ts (cluster and *redis files are excluded from the local project again, batcher from the redis projects) Known issue unchanged: cluster-coordination 'pass messages after Group re-instantiations' flake (documented in .context/ts-migration-plan.md)
- Drop allowJs (no JS remains in the checked tree; fixtures stay .mjs) - Enable noUnusedLocals/noUnusedParameters; rename intentionally unused params to _-prefixed - Update doc references from .js to .ts test/source paths (README, AGENTS.md)
- Tag every underscore-prefixed member (fields and methods, incl. the __-prefixed datastore API) across src with /** @internal */ and enable stripInternal in tsconfig: rolldown-plugin-dts honors it, so dist/index.d.mts/.d.cts and dist/light.d.ts no longer leak internals (verified: zero underscore members in the generated declarations) - In-repo type checking is unaffected (@internal only affects declaration emit), so the suite's legitimate use of internals (_submitLock, _store, _states, ...) keeps compiling - test/bottleneck.ts: the bridge now casts the dist entrypoints through the src class type — dist dts (stripped) and source types (unstripped) legitimately differ on internal members, so the structural comparison at that seam is meaningless
Found by retroactively diffing the old hand-written bottleneck.d.ts (16f94eb^) against dist/index.d.mts: Events installs on/once at runtime on the connection instances and the old contract declared them, but the generated dts omitted them. Declared as typed members ('error' event + string fallback).
- Tag 47 runtime-public members that were never part of the published contract or README (Bottleneck: *Defaults objects, version, Promise, rejectOnDrop/trackDoneStatus, channel_client; Group: instances, interval, limiterOptions, sharedConnection, ...; Batcher: maxTime/ maxSize/options/Events; connections: Redis/client/subscriber/limiters/ shas/terminated/...) with @internal so stripInternal keeps them out of the generated dts - Restore the discriminated connection-option unions from the old contract (Redis xor client, per connection type) as RedisConnectionOptions/IORedisConnectionOptions in src/types.ts, re-exported from the package root; the ctors now accept them and RedisDatastore no longer passes the unused Promise key - types.test.ts asserts exclusivity via @ts-expect-error (Redis+client must not compile) Found by diffing the old hand-written contract against the generated dts; verified zero unintended deltas remain.
Replace the let ExportedBottleneck! + if/else with a makeTestBottleneck() function that returns the wrapped class, so both paths are symmetric and the definite-assignment assertion is gone. No behavior change.
The typed conversion dropped the await before subscriber.unsubscribe(channel) in both RedisConnection and IORedisConnection, so Group.deleteKey()/disconnect() returned before the UNSUBSCRIBE was processed server-side. A limiter re-created on the same channel (Group re-instantiation) then raced the in-flight UNSUBSCRIBE: node-redis v6 silently skips issuing SUBSCRIBE when a listener entry for the channel still exists, so the channel ended up unsubscribed and the new limiter never received messages — the cluster-coordination 'pass messages after Group re-instantiations' flake (was ~50% on this branch, 0/10 failures after this fix; main was green only because its CJS code had the await). Diagnosed with temporary instrumentation inside @redis/client (since removed): PUBSUB-UNSUB -> subscribe-skip -> PUBSUB-DELETE-ENTRY ordering captured the race directly.
Second dropped-await from the conversion, same family as cbb1efd: subscriber.subscribe(channel, cb) was not awaited, so __addLimiter__ resolved before the subscription was confirmed and the subscribe promises were left unhandled — rejected by destroy()'s flushAll during teardown with 'Disconnects client', surfacing as unhandled rejections that failed the vitest run exit code (14 in a full suite). Also: - dedupe doubled /** @internal */ tags on __-methods - types.test.ts: connection fakes satisfy _setup/__addLimiter__ sync and async surface (setMaxListeners/on/once/duplicate) so the test no longer leaks unhandled rejections Full suite: 56 files / 568 passed / 0 unhandled errors.
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
1 participant
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.
No description provided.