types: declare RedisClient.pubsub() and RedisClient.select() - #39208
types: declare RedisClient.pubsub() and RedisClient.select()#39208robobun wants to merge 1 commit into
Conversation
Both methods have been registered on the prototype since Bun.redis was introduced, but redis.d.ts never declared them, so TypeScript rejected calls that work at runtime. pubsub() gets one overload per PUBSUB subcommand whose reply shape is fixed (CHANNELS, NUMSUB, NUMPAT, SHARDCHANNELS, SHARDNUMSUB) plus an untyped catch-all; select() takes the index as a number or string, which is what the runtime coerces. The redis fixture gains assertions for both, and bun-types.test.ts now type-checks that fixture with tsc so the assertions also run on debug builds, where the LanguageService cases are skipped.
|
Warning Review limit reached
Next review available in: 28 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
Comment |
|
Updated 3:09 PM PT - Aug 15th, 2026
✅ @robobun, your commit 551d0ca696f1bc1381b05d7a6cab58df935cfea9 passed in 🧪 To try this PR locally: bunx bun-pr 39208That installs a local version of the PR into your bun-39208 --bun |
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Since packages/bun-types/ has a CODEOWNER, leaving final sign-off on the overload shapes and JSDoc wording to them.
What was reviewed:
- Confirmed
pubsubandselectare registered on the prototype (valkey.classes.ts:568,580) viacmd_strings_varargs!with the statedNotSubscriber/DontCareflags. - Return types match the RESP replies and existing conventions (
Promise<"OK">likerename(),Promise<any>fallback likesend()); separate overloads instead ofpattern?: stringcorrectly reflect that the runtime rejects explicitundefined. - The
bun-types.test.tsrefactor is a behavior-preserving extraction of the existingBun.mmaptsc check intoexpectTscToAccept, reused for the new fixture.
Extended reasoning...
Overview
This PR adds TypeScript declarations for two RedisClient methods that already exist at runtime but were never declared: select(index) and pubsub(subcommand, ...). It touches three files: packages/bun-types/redis.d.ts (the declarations plus a one-line addition to the subscribe() JSDoc), test/integration/bun-types/fixture/redis.ts (type-level assertions for every overload and @ts-expect-error cases), and test/integration/bun-types/bun-types.test.ts (extracts the existing Bun.mmap tsc-spawn check into a reusable expectTscToAccept helper and adds a second caller for the redis fixture).
Security risks
None. This is a .d.ts-only change with a compile-time test; no runtime code paths are touched.
Level of scrutiny
Low-to-medium. The declarations themselves are mechanical and well-researched — I verified against src/runtime/valkey_jsc/valkey.classes.ts and js_valkey_functions.rs that both methods exist, that select is NotSubscriber and pubsub is DontCare, and that cmd_strings_varargs! stringifies numbers and rejects undefined (justifying number | string for select and separate overloads over optional params). The return types ("OK", string[], (string | number)[], number, any) match the Redis PUBSUB/SELECT reply shapes and follow the file's existing conventions (rename() returns Promise<"OK">, send() returns Promise<any>). The test refactor is a straight extraction with no behavior change to the mmap case.
Other factors
packages/bun-types/ is CODEOWNER'd, and per the approval guidelines that means a human should sign off rather than auto-approving. The overload design (uppercase-literal subcommands with a string catch-all, JSDoc noting the reconnect caveat for select()) is user-facing API surface where the owner may have preferences on shape or wording. Nothing in the change looks wrong; deferring solely on the CODEOWNER rule.
|
Status: ready for review (CI green: the Reproduced with the released Bun 1.4.0: Self-review done. No findings against the declarations or JSDoc. It did note that the |
…s tables (#39271) ### Problem - `class RedisClient` in `packages/bun-types/redis.d.ts` is a hand-written copy of the `proto` table in `src/runtime/valkey_jsc/valkey.classes.ts`, and nothing compares the two. A command added to the table and not to the d.ts works at runtime and fails to type-check (`Property 'pubsub' does not exist on type 'RedisClient'`). - On main, 5 of the 167 table entries are undeclared: `psubscribe`, `pubsub`, `punsubscribe`, `script`, `select`. All five have been registered since the commit that introduced `Bun.redis` (ec87a27, #18812) and stayed undeclared through the 14 redis.d.ts commits since, including the 614-line batch in #23116, so the gap is systematic rather than a one-off. - Each of the five is already being declared by an open PR: `pubsub` and `select` by #39208, `script` by #29339, `psubscribe`/`punsubscribe` by #35521 (which also adds the listener routing they are missing; today `psubscribe()` drops every pmessage). This PR adds the check, not the declarations. ### Fix - `test/internal/source-lints/redis-client-types.test.ts` imports `valkey.classes.ts` (the module the codegen reads; `class-definitions.ts` has no dependencies), collects the members the codegen installs (`proto` entries by name, `klass` entries as `static name`, `constructor` when `construct` is set, skipping `internal`/`privateSymbol`/`publicSymbol` entries, which the codegen does not install under an identifier), parses the member names declared in the `class RedisClient` body of `redis.d.ts` (`[Symbol.x]` maps onto the table's `@@x` spelling), and requires the two sets to be equal. Each direction is its own test, so a table entry without a declaration and a declaration without a table entry are both reported by name. - The five names missing today sit in a `pendingDeclarations` table keyed by the PR that declares each. A fourth test fails as soon as a listed name is declared or unregistered, so the entry gets deleted when its PR lands (whichever of this PR and #39208/#29339/#35521 lands second trips it on rebase, and the message says which entry to delete). A name that is neither declared nor listed fails the lint outright. - Declaration lines the parser does not recognize are reported as failures rather than skipped, so a new d.ts shape cannot silently hide a member from the comparison. - `.github/workflows/source-lints.yml` gains `src/**/*.classes.ts`, `src/codegen/class-definitions.ts` and `packages/bun-types/redis.d.ts` as triggers: the workflow is path-filtered and those are the files this lint reads, so without them the edits it guards would not run it. The directory README now states that rule; a comment on the `proto` table points at the lint. - Scope is RedisClient only. A checker for every `*.classes.ts` needs a class-to-declaration mapping and inheritance handling and is a separate project. - Verified: - `bun test test/internal/source-lints/redis-client-types.test.ts` passes on this branch (4 tests); the whole directory is 170 green. - With `pendingDeclarations` emptied, the lint fails against main's files naming exactly `psubscribe`, `pubsub`, `punsubscribe`, `script`, `select` (output below). - Simulated the eight drift shapes in the details block (new table entry, pending name declared, phantom declaration, pending name unregistered, unparseable member, `@@asyncDispose` + a `klass` entry with and without declarations, `internal`/`privateSymbol` entries); each fails the intended test or passes as intended. - No runtime code changes: the `valkey.classes.ts` hunk is a comment and produces identical codegen output. This PR has no src/packages diff for a fail-before run to strip; the fail-before evidence is the emptied-pending-table run above. ### Background - `*.classes.ts` files are the input of `src/codegen/generate-classes.ts`. `define({ proto, klass, construct })` describes a native class: `proto` entries become properties of the prototype (`fn` methods, `getter`/`setter` accessors), `klass` entries become statics, and `construct: true` makes it newable. Entries with `internal`, `privateSymbol` or `publicSymbol` are installed under private names or `Symbol.for()` symbols (or not at all), and keys spelled `@@x` are installed under the well-known symbol `Symbol.x`. - `packages/bun-types` is the published `@types/bun` surface; it is hand-written, not generated from the class definitions, which is why it can drift. - `test/internal/source-lints/` holds tests that only read the source tree; `.buildkite/ci.mjs` excludes the directory from the binary lanes and `source-lints.yml` runs it against a released bun on a bare checkout, so tests there may only import built-ins and relative paths. <details> <summary>Lint output against main's files with the pending table emptied, and the simulated drift shapes</summary> ``` (pass) every member of class RedisClient in packages/bun-types/redis.d.ts has a shape this lint can read (fail) packages/bun-types/redis.d.ts declares every RedisClient member src/runtime/valkey_jsc/valkey.classes.ts installs - [] + [ + "psubscribe", + "pubsub", + "punsubscribe", + "script", + "select", + ] (pass) packages/bun-types/redis.d.ts declares no RedisClient member src/runtime/valkey_jsc/valkey.classes.ts does not install (pass) pendingDeclarations lists only members that are still registered and still undeclared ``` ``` ### proto gains waitaof, d.ts untouched + "waitaof", (fail) packages/bun-types/redis.d.ts declares every RedisClient member src/runtime/valkey_jsc/valkey.classes.ts installs ### d.ts declares select while it is still pending + "select (#39208) is declared in packages/bun-types/redis.d.ts now; delete its entry", (fail) pendingDeclarations lists only members that are still registered and still undeclared ### d.ts declares flushall, which is not registered + "flushall", (fail) packages/bun-types/redis.d.ts declares no RedisClient member src/runtime/valkey_jsc/valkey.classes.ts does not install ### proto drops script while it is still pending + "script (#29339) is no longer registered in src/runtime/valkey_jsc/valkey.classes.ts", (fail) pendingDeclarations lists only members that are still registered and still undeclared ### d.ts gains `private brand: never;` + "private brand: never;", (fail) every member of class RedisClient in packages/bun-types/redis.d.ts has a shape this lint can read ### proto gains "@@asyncDispose" and a klass entry, d.ts declares [Symbol.asyncDispose]() and the static 4 pass ### same, with nothing declared + "@@asyncDispose", + "static parseURL", (fail) packages/bun-types/redis.d.ts declares every RedisClient member src/runtime/valkey_jsc/valkey.classes.ts installs ### proto gains an `internal: true` entry and a `privateSymbol` entry, d.ts untouched 4 pass ``` </details>
Problem
RedisClient.prototype.pubsubandRedisClient.prototype.selectwork at runtime, butpackages/bun-types/redis.d.tsdoes not declare either, sotscreportsProperty 'pubsub' does not exist on type 'RedisClient'(same forselect) on calls that succeed when run.Bun.rediswas introduced (src/runtime/valkey_jsc/valkey.classes.ts:568and:580, implemented bycmd_strings_varargs!atsrc/runtime/valkey_jsc/js_valkey_functions.rs:1606and:1710); the declarations were never added.valkey.classes.ts, these are the only two undeclared ones that are not already covered by an open PR (scriptis in redis: add bitmap, HLL, geo, scripting, server, and stream commands #29339;psubscribe/punsubscribeare in redis: implement psubscribe/punsubscribe with listener routing #35521, which also adds the missing message delivery they need).Fix
select(index: number | string): Promise<"OK">. The runtime stringifies numbers (from_jsinjs_valkey_functions.rs:70), andSELECTreplies with the simple stringOK, the same replyrename()andset()already type as"OK".pubsub(...): one overload per subcommand whose reply shape is fixed, typed from the replies observed against Redis 8.0.2 (CHANNELSandSHARDCHANNELSgivestring[],NUMSUBandSHARDNUMSUBgive a flat(string | number)[]alternating name and count,NUMPATgives anumber), pluspubsub(subcommand: string, ...args: string[]): Promise<any>for everything else (HELP, lowercase spellings, future subcommands), mirroring thePromise<any>thatsend()already uses for untyped replies.CHANNELS [pattern]) are separate overloads rather thanpattern?: string, matching the rest of the file:cmd_strings_varargs!throwsERR_INVALID_ARG_TYPEon an explicitundefined, and an optional parameter would accept it.pubsub()may be called while in subscriber mode (it is registered withDontCare),select()may not (NotSubscriber), and a database chosen withselect()is lost on automatic reconnect while one given in the URL path is re-selected. Thesubscribe()JSDoc list of commands usable while subscribed gainspubsub()so it matches the new declaration.test/integration/bun-types/fixture/redis.tsasserts the exact return type of every overload withexpectType(...).is<...>()and uses@ts-expect-errorfor the call shapes the runtime rejects (no subcommand, explicitundefined, extra argument toselect). The fixture is already covered in CI by the LanguageService cases (this file runs in thebun-typesGitHub workflow on a release Bun).bun-types.test.tsadditionally gets aRedisClientcase that type-checks that fixture with spawnedtsc, sharing a helper with the existingBun.mmapcase, so the assertions also run underbun bd test, where the LanguageService cases are skipped; it is the same shape as theBun.mmapcase from types: add offset and size options to Bun.mmap #34573 and can go once the file checks the whole fixture on debug builds (tracked separately).bun test test/integration/bun-types/bun-types.test.tswith the system Bun: 10 of 16 cases fail without theredis.d.tschange (15TS2339diagnostics, under both the DOM and no-DOM configs), 16 pass with it.bun bd test test/integration/bun-types/bun-types.test.ts: the newRedisClientcase fails without theredis.d.tschange and passes with it.Background
valkey.classes.tsis the list of methods generated ontoRedisClient.prototype;redis.d.tsis maintained by hand, so a method can exist at runtime without a declaration.cmd_strings_varargs!is the macro behind both methods: it forwards every argument to the server as a string or buffer (numbers are stringified first) and throwsERR_INVALID_ARG_TYPEfor anything else, includingundefined. Argument count and subcommand validity are left to the server.subscribe(). Each method is registered asNotSubscriber(rejected in that state) orDontCare(allowed);pubsubisDontCare,selectisNotSubscriber.bun-types.test.tstype-checks thefixture/directory with the in-process TypeScript LanguageService, but those cases areskipIf(isDebug)because that is very slow on debug builds; Buildkite does not run this file at all (.buildkite/ci.mjsexcludesintegration/bun-types), only the GitHub workflow does. Spawningtscover a couple of files is the existing workaround for checks that should also run under a debug build.Runtime probe (system Bun 1.4.0, local Redis 8.0.2)