bun-types: declare the Error base and the runtime members of BuildMessage and ResolveMessage - #38564
bun-types: declare the Error base and the runtime members of BuildMessage and ResolveMessage#38564robobun wants to merge 1 commit into
Conversation
…sage and ResolveMessage Both classes put Error.prototype in their chain and expose line, column, toString(), toJSON() and Symbol.toPrimitive; BuildMessage also has notes, ResolveMessage also has stack and requireStack. The declarations in globals.d.ts listed none of these, so reading them was a type error. The hand-maintained copies of the declarations in docs/bundler/index.mdx are updated to match. The bun-types test builds two files, checks the runtime shape, and type-checks code that uses exactly that shape; the fixture pins the exact member types under every tsconfig it is run with.
|
Warning Review limit reached
Next review available in: 37 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 (4)
Comment |
|
Status: types-only change, ready for review. Automated review found nothing to change; CI: the Buildkite build for 40eb383 is green on all 177 jobs that ran. The only two jobs that did not pass are the two Reproduced by running the API on bun 1.4.0 and a debug build of main: both classes are Verification:
Related: #38542 fixes the |
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Deferring to a human because packages/bun-types/ is CODEOWNER-protected.
What was reviewed:
- Each new member (
line,column,notes,stack,requireStack,toJSON,toString,Symbol.toPrimitive,extends Error) traced to its.classes.tsproto entry and Rust getter — types and JSDoc match what the getters return, including the zero-based line/column and the exacttoJSONkey sets. ResolveMessage.stack: string(non-optional, mutable) matchesget_stackalways returning a string and thewritable: trueaccessor;BuildMessagecorrectly inherits the optionalError.stackinstead.- The refactored
expectToTypecheckhelper preserves the existingBun.mmapcase's behavior; the new runtime-shape assertions and fixture@ts-expect-errorguards are non-vacuous.
Extended reasoning...
Overview
This PR adds missing member declarations to the global BuildMessage and ResolveMessage classes in packages/bun-types/globals.d.ts: extends Error, line/column, notes, stack, requireStack, toString(), toJSON(), and [Symbol.toPrimitive]. It updates two hand-written copies in docs/bundler/index.mdx to match, extracts an expectToTypecheck helper in test/integration/bun-types/bun-types.test.ts (reused by the existing Bun.mmap case), adds a runtime+tsc test case for the new members, and extends test/integration/bun-types/fixture/build.ts with expectType pins.
Security risks
None. Pure .d.ts additions, docs, and test fixtures — no runtime code path changes.
Level of scrutiny
Medium-low for correctness (types-only, purely additive, no existing declarations narrowed), but packages/bun-types/ has a CODEOWNER entry, which per the approval guidelines means a human should sign off. I cross-checked every new declaration against src/jsc/resolve_message.classes.ts, src/jsc/BuildMessage.rs, and src/jsc/ResolveMessage.rs:
prototypeBase: "Error"on both →extends Erroris accurate.get_line/get_columnreturnlocation.{line,column} - 1or0→number, zero-based JSDoc is correct.get_noteswraps each note as aBuildMessagewithKind::Note→BuildMessage[]is correct.get_require_stackreturnsUNDEFINEDunlessImportKind::Require | RequireResolve, else a string array →string[] | undefinedis correct.get_stack(ResolveMessage only) always returns a string;writable: truein the classes.ts → mutablestack: stringis correct.BuildMessagehas nostackin its proto, so inheritingError's optionalstack?: stringis the right shape.to_jsonwrites exactly the keys thePick<...>types name.to_primitivereturns the toString result for"default"/"string"hints andNULLotherwise → the two-overload signature is accurate.
Other factors
The refactor of the mmap test into expectToTypecheck is behavior-preserving (same tsconfig setup, same spawn, same assertions). The new test case both asserts runtime shape and drives tsc against the packed declarations, and the fixture adds @ts-expect-error guards so over-declaring would also fail. The docs change fixes a factual error (ResolveMessage extends BuildMessage was wrong; instanceof disproves it). The only reason not to auto-approve is the CODEOWNER on packages/bun-types/.
Problem
declare class BuildMessageanddeclare class ResolveMessagein packages/bun-types/globals.d.ts declare 4 and 9 members and no base class. The runtime objects (src/jsc/resolve_message.classes.ts, getters in src/jsc/BuildMessage.rs and src/jsc/ResolveMessage.rs) haveError.prototypein their chain (prototypeBase: "Error") and also exposeline,column,toString(),toJSON()andSymbol.toPrimitiveon both,notesonBuildMessage, andstackandrequireStackonResolveMessage.Bun.build()logs or a caught import/require failure is a type error while working at runtime:log.line,log.column,log.stack,log.notes,err.requireStack,log.toJSON()all fail with TS2339 (Property 'line' does not exist on type 'BuildMessage | ResolveMessage').Fix
extends Errorand declare the members above. Every declared member is one the runtime defines, with the type the getter returns:line/column:number, documented as zero-based (get_line/get_columnreturnlocation.line - 1, and0without a location), unlike the one-basedposition.line/position.column.BuildMessage.notes:BuildMessage[](get_noteswraps each note as aBuildMessageof kind note).ResolveMessage.stack:string, mutable like the runtime accessor (it has a setter) and likeError.stack.BuildMessagegets nostackof its own because the runtime defines none there; the inherited optionalError.stackis what it actually has (undefined).ResolveMessage.requireStack:string[] | undefined. The getter always exists and returnsundefinedfor anything but a failedrequire()/require.resolve(), so it is declared as always present rather than optional.toJSON():Pick<...>of exactly the keys the runtimeto_jsonwrites (name,position,message,level, plusspecifier,importKind,referreronResolveMessage).Pickkeeps it in sync with the member declarations, including thelevel/importKindunions being corrected in bun-types: type BuildMessage/ResolveMessage level and importKind as the strings the runtime reports #38542.[Symbol.toPrimitive]:stringfor the"default"/"string"hints,string | nullotherwise, which is whatto_primitivedoes.levelandimportKindliteral unions are wrong too but are fixed by bun-types: type BuildMessage/ResolveMessage level and importKind as the strings the runtime reports #38542; those lines are left untouched here so the two changes compose. ResolveMessage/BuildMessage: add .stack property #35632 adds a realstacktoBuildMessageat runtime and would tighten the inherited declaration when it lands.ResolveMessageas extendingBuildMessage, which it does not:resolveMessage instanceof BuildMessageisfalse).requireStackforrequire()only,toJSON()keys,toPrimitiveresults), then type-checks code using exactly that shape against the packed declarations with tsc. The tsc helper is shared with the existingBun.mmapcase and runs on debug builds too. test/integration/bun-types/fixture/build.ts pins the exact member types and is checked under each tsconfig the file runs (no lib, DOM, tsgo) on release builds.bun test(release, 16 pass) andbun bd test(debug).Background
Bun.build()returns each bundler diagnostic inlogsas aBuildMessage, or aResolveMessagewhen it came from failing to resolve an import. A failedimport/import()/require()at runtime throws the same classes..classes.tsdefinitions: every member listed underprotothere is a getter or method on the prototype, andprototypeBase: "Error"putsError.prototypebehind that prototype, which is whyinstanceof Errorholds but the constructor-side statics ofErrorare not involved.requireStackmirrors the property Node.js puts on itsMODULE_NOT_FOUNDerrors; Bun records only the direct caller, so it is a one-element array.Runtime probe (bun 1.4.0, same on a debug build of main)
tsc over the same property accesses with the declarations from main: