fix(instrument-bundler): keep the vendored esbuild module out of tree shaking - #1493
Closed
joshunrau wants to merge 3 commits into
Closed
Conversation
… shaking `sideEffects: ["**/cli.ts"]` marks every other module in the package as pure, but the whole body of `vendor/esbuild.ts` is a side effect: a conditional top-level `await import()` assigning to hoisted `var`s. esbuild therefore drops it from any consumer bundle while still inlining the namespace access down to the bare `build` identifier, so every call throws `ReferenceError: build is not defined`. This is why `serve-instrument@2.2.0` cannot compile any instrument at all — its published `dist/cli.js` contains no reference to esbuild whatsoever. That failure was invisible because the catch branches in `build` were inverted: a `ReferenceError` does not satisfy `$BuildFailure`, so the parse failed and the ZodError describing the schema mismatch was thrown as the cause of `Failed to Compile`, discarding the real error. Meanwhile a genuine esbuild failure, which does parse, was reported as `Unknown Error` with no `kind`, so `InstrumentErrorFallback` never rendered a code frame for it. Report an esbuild failure as `Failed to Compile` / `ESBUILD_FAILURE`, passing the original error rather than the parsed copy so `cause instanceof Error` still holds downstream, and name any other error in the message while keeping it as the cause. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Collaborator
Author
|
Closing in favour of #1482, which fixes the same root cause and was opened first. I found this from #1482 is the better fix on every axis this PR touches:
The two deltas from this PR worth keeping — passing the original error rather than the parsed copy as the |
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
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.
Problem
serve-instrument@2.2.0cannot compile any instrument. Every request fails with a ZodError that has nothing to do with the instrument being served:The real error is
ReferenceError: build is not defined, thrown at theesbuild.build(...)call inbuild.ts. The publisheddist/cli.jsfor 2.2.0 contains zero occurrences of the stringesbuild(2.1.4 has two).Cause
2.2.0 added
"sideEffects": ["**/cli.ts"]topackages/instrument-bundler/package.json. That marks every other module in the package as pure, but the entire body ofsrc/vendor/esbuild.tsis a side effect — a conditional top-levelawait import()assigning to hoistedvars:esbuild drops that module from the consumer bundle while still inlining the namespace access down to the bare
buildidentifier, leaving a dangling reference.Rebuilding
packages/serve-instrumentagainst each variant:sideEffectsesbuildrefs indist/cli.js["**/cli.ts"](current)ReferenceErroron every bundle["**/cli.ts", "**/vendor/esbuild.ts"](this PR)Why it surfaced as a ZodError
The catch branches in
buildwere inverted. AReferenceErrordoes not satisfy$BuildFailure, so the parse failed and the ZodError describing the schema mismatch was thrown as the cause ofFailed to Compile, discarding the actual error. Symmetrically, a genuine esbuild failure — which does parse — was reported asUnknown Errorwith nokind, soInstrumentErrorFallbacknever rendered a code frame for real syntax errors.Changes
package.json: add**/vendor/esbuild.tstosideEffects.build.ts: report an esbuild failure asFailed to Compile/ESBUILD_FAILURE, passing the original error rather than the parsed copy socause instanceof Errorstill holds forInstrumentErrorFallbackandcause.errors[0].locationstill reachesCodeErrorBlock; name any other error in the message and keep it as the cause.vendor/esbuild.ts: comment recording thesideEffectscoupling, since nothing else in the source hints at it.Verification
pnpm lintclean; all 42 existinginstrument-bundlertests pass.serve-instrument:dist/cli.jscontains the esbuild import again and compiles a real multilingual form instrument (a.tsxentrypoint with a JSX block) that 2.2.0 rejects.Failed to Compile | kind: ESBUILD_FAILURE | cause.errors[0]: Expected identifier but found end of fileinstead ofUnknown Error.esbuild.buildto reject with aReferenceError: message becomesUnexpected error while invoking esbuild: ReferenceError: build is not defined, with the original error ascause.Worth a 2.2.1 — 2.2.0 is unusable for local instrument development as published.
🤖 Generated with Claude Code