Conversation
- Remove the static `Worker.data` from the constructor declaration. The runtime has no such property (`"data" in Worker` is false). - `WorkerOptions.ref` defaults to true, as docs/runtime/workers.mdx says and the runtime does. The JSDoc said false. - Give the worker-global `postMessage` the same two overloads as `Worker.prototype.postMessage`: a transfer list or a StructuredSerializeOptions bag. The runtime accepts both.
|
Warning Review limit reached
On-demand reviews are free for the next 13 days. After that, they cost $0.25 per reviewed file. Or wait 57 minutes for your next included review. View limit detailsLimit details: You’ve used all 10 included reviews currently available. Review configuration: ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (3)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed this and didn't find any bugs — the declarations now match the runtime. Deferring only because packages/bun-types/ is CODEOWNER-gated.
What was reviewed:
refdefault:WorkerOptions.hinitializesbool unref { false }, so workers are ref'd by default —@ default trueis correct.- Global
postMessage:jsFunctionPostMessageinWorker.cppbranches onhasIteratorMethodvsconvertDictionary<StructuredSerializeOptions>, so both overload shapes are accepted at runtime; the new declaration mirrorsWorker.prototype.postMessageexactly. - Fixture under lib.dom: the
@ ts-expect-erroronWorker.datastill errors (lib.dom'sWorkervar has nodataeither), and the threepostMessagecalls resolve via merged overloads, so the existing exact-match diagnostics list for"checks with lib.dom.d.ts"stays valid.
Extended reasoning...
Overview
This PR touches three files: two .d.ts declarations in packages/bun-types/ (JSDoc @ default fix on WorkerOptions.ref, removal of a fictional static Worker.data, and splitting the worker-global postMessage into the standard two-overload pair) plus new compile-time assertions in the existing test/integration/bun-types/fixture/worker.ts. No runtime code is touched.
Security risks
None. Type declarations and JSDoc only; nothing here affects execution, parsing, or any trust boundary.
Level of scrutiny
Low-to-moderate. Per .claude/docs/landing-prs.md, .d.ts changes must mirror runtime exactly, so I traced each claim into C++: src/jsc/bindings/webcore/WorkerOptions.h:21 shows bool unref { false } (workers ref'd by default → @ default true is correct); JSWorker.cpp defines no static data on the constructor table; and jsFunctionPostMessage in Worker.cpp:436-447 accepts both a sequence and a StructuredSerializeOptions dictionary as arg 2, matching the new overload pair. The new overloads are byte-identical in shape to the existing Worker.prototype.postMessage pair at bun.d.ts:716-717, and Bun.Transferable/Bun.StructuredSerializeOptions already exist. I also checked the integration test's "checks with lib.dom.d.ts" case, which asserts an exact diagnostics array including worker.ts entries — the new fixture lines produce no additional diagnostics under lib.dom (global declare function postMessage overloads merge with lib.dom's, and lib.dom's Worker var also lacks .data, so the @ ts-expect-error remains satisfied).
Other factors
.github/CODEOWNERS assigns *.d.ts and /packages/bun-types/ to a code owner, which per the approval guidelines means I should not auto-approve even when confident the change is correct. The change is otherwise small, self-contained, covered by fixture assertions (including a negative @ ts-expect-error that will fail if Worker.data is ever re-added without runtime support), and follows the existing test-organization convention of extending the feature's fixture file rather than creating a new one.
|
Updated 6:48 PM PT - Sep 7th, 2026
❌ @Jarred-Sumner, your commit 695f495 has 5 failures in
🧪 To try this PR locally: bunx bun-pr 41818That installs a local version of the PR into your bun-41818 --bun |
|
Status: ready for review. Verified with CI build 112038: the red tests ( |
… with @types/node 26.5 (#41884) Test expectation only. No types or runtime change. ### Problem - The `bun-types` workflow ("TypeScript types") fails on every PR since 2026-09-07 14:07 UTC. The case `lib configuration > checks with lib.dom.d.ts` expects TS2322 at `24154.ts:11:3` (the `stream()` return types differ). It now gets TS2741: `Property 'textStream' is missing in type 'Blob' but required in type 'import("node:buffer").Blob'.` - The fixture resolves `@types/node` to `latest`. `@types/node@26.5.0`, published at that time, added `textStream()` to `node:buffer`'s `Blob` (Node.js v24.19.0 / v26.5.0). lib.dom's `Blob` has no `textStream`, so TypeScript reports the missing property first. ### Fix - Update the expected diagnostic for `24154.ts:11:3` in the lib.dom case to the TS2741 message. - bun-types stays as it is. Bun's `Blob` has no `textStream()` at runtime (only `Request` and `Response` do, `fetch.d.ts:81,93`), so declaring it on the global `Blob` would be wrong. The no-lib.dom cases still pass: there `Response#blob()` already returns `node:buffer`'s `Blob`. - Verified: `bun test test/integration/bun-types/bun-types.test.ts`, 21/21 with `@types/node@26.5.0`. On main the lib.dom case fails with the diff above. ### Background - `fixture/24154.ts` returns `await response.blob()` from a function typed `Promise<import("node:buffer").Blob>`. Under lib.dom, `Response#blob()` yields lib.dom's `Blob`, which is not assignable to node's. The test pins the exact text of that one expected diagnostic. - BuildKite excludes `integration/bun-types`. Only `.github/workflows/bun-types.yml` runs this file, on PRs that touch `packages/bun-types` or `test/integration/bun-types`. <details><summary>Notes</summary> - `@types/node` publish times: 26.4.1 at 2026-09-01 (no `textStream`), 26.5.0 at 2026-09-07T14:07:47Z (adds it at `buffer.d.ts:1779`). Every `bun-types.yml` run after that is red across unrelated branches, for example #41809, #41818, #41708. - Node.js documents `blob.textStream()` as added in v26.5.0 and v24.19.0. Implementing it on Bun's `Blob` is a separate runtime change. It would not alter this lib.dom diagnostic unless bun-types also declared it on the global `Blob`. - The type-checking cases in this file are `test.skipIf(isDebug)`, so a debug build skips them. CLAUDE.md says to run this file with the system bun, which is what the verification above did. </details> <!-- robobun:evidence:begin --> --- **[auto-merge]** gate passed · iteration 0 · 1 files touched <details><summary>passes on PR (with fix)</summary> ```console Test-only change. Debug/ASAN (expected pass): $ bun bd test 'test/integration/bun-types/bun-types.test.ts' $ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test test/integration/bun-types/bun-types.test.ts bun test v1.4.3 (f42e980) test/integration/bun-types/bun-types.test.ts: (pass) @types/bun integration test > building and packing bun-types leaves packages/bun-types untouched [4.75ms] (pass) @types/bun integration test > packed bun-types includes CLAUDE.md [10.62ms] (skip) @types/bun integration test > basic type checks > checks without lib.dom.d.ts (skip) @types/bun integration test > TypeScript latest > checks without lib.dom.d.ts (skip) @types/bun integration test > TypeScript 7.1 > checks the fixture and import attributes through ts7.1/index.d.ts (pass) @types/bun integration test > Bun.mmap > MMapOptions accepts offset and size [754.01ms] (pass) @types/bun integration test > TextDecoder > accepts the encoding labels the runtime supports [773.13ms] (pass) @types/bun integration test > TextDecoder > the fixture label table matches the runtime [40.96ms] (pass) @types/bun integration test > Event and EventTarget > lib.dom's composedPath() declaration wins when lib.dom is loaded [910.82ms] (pass) @types/bun integration test > Event and EventTarget > the Node-style composedPath() tuple applies without lib.dom [824.53ms] (pass) @types/bun integration test > process event methods with @types/node@24 > removeListener and off accept other event names [2035.99ms] (skip) @types/bun integration test > Test Globals > checks without lib.dom.d.ts and test-globals references (skip) @types/bun integration test > Test Globals > test-globals FAILS when the test-globals.d.ts is not referenced (skip) @types/bun integration test > bun:bundle feature() > Registry augmentation restricts feature() to known flags (skip) @types/bun integration test > bun:bundle feature() > Registry augmentation produces type errors for invalid flags (skip) @types/bun integration test > bun:bundle feature() > without Registry augmentation, feature() accepts any string ... (truncated) Exit: 0 ``` </details> <details><summary>diff hotspot</summary> ``` test/integration/bun-types/bun-types.test.ts | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) ``` </details> **gate history** · 1 passed · 0 rejected · iteration 0 <details><summary>evidence per changed file</summary> ``` file reads edits tests test/integration/bun-types/bun-types.test.ts 2 1 6 ``` </details> <!-- robobun:evidence:end --> --------- Co-authored-by: Alistair Smith <hi@alistair.sh>
This branch has not been deployed
2 participants
Problem
globals.d.tsdeclares a staticWorker.data: any("Bun's equivalent ofworkerData"). The runtime has no such property:"data" in WorkerisfalseandWorker.dataisundefinedon 1.4.2 and canary.Bun.WorkerOptions.refis documented as@default false. The runtime refs workers by default, and docs/runtime/workers.mdx says "Workers are ref'd by default".declare function postMessage(message, transfer?)lacks theStructuredSerializeOptionsoverload.postMessage(msg, { transfer: [ab] })works at runtime (the buffer is detached) but fails to type-check without lib.dom.Fix
datamember from theWorkerconstructor declaration.refJSDoc to@default trueand note thatref: falseequalsworker.unref().postMessage(message, transfer: Transferable[])andpostMessage(message, options?: StructuredSerializeOptions), the same pair asWorker.prototype.postMessageand lib.webworker.bun test test/integration/bun-types/bun-types.test.ts(21 pass). The new fixture lines fail against the old declarations ("'transfer' does not exist in type 'Transferable[]'" and "Unused '@ts-expect-error' directive").Background
dataoption tonew Worker()does exist at runtime as an alias forworkerData(read inJSWorker.cpp), and surfaces inside the worker asrequire("node:worker_threads").workerData. Only the static accessor was never implemented. PassingdatatoWorkerdoes not work #9330 asks for it and Worker: implement static Worker.data (Bun's workerData alias) #35572 implements it. If that lands, it re-adds the declaration with a working runtime behind it.