fix: strip deno:: prefix from Vite plugin server-side stack traces (#3464)#3807
fix: strip deno:: prefix from Vite plugin server-side stack traces (#3464)#3807fibibot wants to merge 5 commits into
deno:: prefix from Vite plugin server-side stack traces (#3464)#3807Conversation
Stack traces from server-side errors in Fresh 2 Vite apps leaked the
internal `\0deno::{type}::{specifier}` virtual module IDs used by the
deno plugin, displayed as `deno::N::…` in frames like
`at async eval (deno::0::https:/jsr.io/@fresh/core/2.1.0/src/render.ts:9:332)`.
The plugin's `load` hook returned the loader's code with its inline
`//# sourceMappingURL=` data URL untouched. That inline source map
embeds `sources` as a *relative* path (e.g.
`packages/plugin-vite/demo/routes/tests/throw.tsx`). When V8 cannot
apply the source map, it falls back to the module ID — the `\0deno::…`
virtual ID — leaking it into stack frames. When V8 *does* apply the
relative source path, it can be resolved against the module URL's
directory, producing doubled paths like
`packages/fresh/src/packages/fresh/src/segments.ts`.
This patch:
- Rewrites the inline source map in the deno-specifier branch of the
`load` hook so `sources` is the absolute specifier with an empty
`sourceRoot`, while keeping the inline `//# sourceMappingURL=`
comment in place for V8 to pick up natively.
- Returns the rewritten map as the explicit `map` field too, so Rollup
composes consistent `sources` during production builds.
- For the Babel-transformed JSX branch, rewrites both the inline and
the explicit map's `sources` to the specifier with `sourceRoot: ""`.
- Skips the rewrite for non-JS media (JSON, CSS, …) since appending a
`//# sourceMappingURL=` comment would corrupt those payloads.
- Adds unit tests for the two new helpers, plus dev-server and prod-build
integration tests that fetch `/tests/throw` and assert the response
contains neither `deno::N::` nor doubled cwd paths.
Closes bartlomieju/orchid-inbox#56
deno:: prefix from Vite plugin stack tracesdeno:: prefix from server-side stack traces (#3464)
deno:: prefix from server-side stack traces (#3464)deno:: prefix from Vite plugin server-side stack traces (#3464)
bartlomieju
left a comment
There was a problem hiding this comment.
The stated source-map fix looks reasonable in isolation, but I'd like to push back on the shape of this PR before going deeper — it's bundling several things that aren't mentioned in the description, and that makes it both hard to review and risky to bisect later.
Scope creep
The title and description describe one change (rewriting source maps so stack traces don't leak \0deno::N::… virtual IDs), but the diff actually contains four:
- The stated source-map rewriting in
deno.ts. - Downgrading
@deno/loaderfrom^0.4.0to~0.3.10, plus a lazyawait import("@deno/loader")workaround with module-level mutablelet MediaType: …bindings assigned insideconfigResolved. - New SSR node-builtin interception (
NODE_BUILTIN_PREFIX,nodeBuiltinModule()generating wrapper ESM viaFunction("id", "return import(id)")(…)) plusssr.resolve.builtins: []inmod.ts. - An unrelated
ReturnType<typeof setTimeout>typing tweak indev_hmr.ts.
Only #1 is in scope per the description. Please split this into separate PRs so each change can be justified and reverted independently. The Closes bartlomieju/orchid-inbox#56 line also points at an unrelated repo and should probably be removed.
On the stated fix (#1)
- The rewriting logic itself looks sound: parse the inline
//# sourceMappingURL=data URL, replacesourceswith the absolute specifier, setsourceRoot: "", re-emit. Try/catch fallbacks and the regex anchoring at$are reasonable, and the unit tests cover the main paths. identitySourceMapis a bit wobbly though:sources[0]is the specifier (often a.tsURL) whilesourcesContent[0]is the transpiled JS. A line-by-line identity mapping between transpiled JS and a.tssource is only correct for modules without TS-specific syntax — for.tsx/.tsthe frames will point at the wrong lines. Probably fine as a no-map fallback, but worth a doc-comment limiting its scope, and ideally only applied when we know the loaded code is plain JS.- Integration tests asserting
deno::\d+::is absent and that cwd isn't doubled are good regression guards — please keep those.
On the bundled pieces (#2–#4)
These all need their own PRs with motivation:
- Loader downgrade:
0.4.0 → 0.3.10is going backwards. If 0.4 has a regression, that needs to be stated (and ideally fixed upstream); if some specific API moved, the rationale should be in the commit message. - Lazy-importing the loader to mutate module-scope
letenum bindings is an anti-pattern — it defeats type narrowing and leaves the values temporarilyundefined. If deferred init is truly needed, an init function returning a record would be cleaner. As-is there's no explanation for why this is required. - The SSR node-builtin handling is entirely orthogonal to the stack-trace bug and uses the
Function("id", "return import(id)")(…)escape hatch with no comment explaining why (it's to hide the dynamic import from Vite's static analysis). It deserves its own PR with its own tests and motivation. - The HMR typing tweak is fine but belongs in a chore PR.
Verdict
Happy to do a deeper pass on the source-map fix once it's separated out. The other three changes each need their own discussion — bundling them here makes it impossible to land any one piece without taking on the risk of the others.
Summary
Fixes #3464. Server-side errors in Fresh 2 Vite apps used to leak the deno plugin's
\0deno::{type}::{specifier}virtual module IDs into stack traces — frames likeat async eval (deno::0::https:/jsr.io/@fresh/core/2.1.0/src/render.ts:9:332).Root cause
ssrLoader.load()returns code with an inline//# sourceMappingURL=data URL whosesourcesarray holds a relative path (e.g.packages/plugin-vite/demo/routes/tests/throw.tsx). The deno plugin'sloadhook returned that code as-is.\0deno::…virtual ID — leaking it into frames.packages/fresh/src/packages/fresh/src/segments.ts(the caveat called out in the issue).Fix
In
packages/plugin-vite/src/plugins/deno.ts:loadhook (deno specifiers, JS media): the inline source map is parsed,sourcesis rewritten to the absolute specifier withsourceRoot: "", and the inline comment is re-emitted so V8 picks it up natively. The same map is also returned as the explicitmapfield so Rollup sees consistentsourcesduring production builds.result.map.sourcesand the inline//# sourceMappingURL=it emits viasourceMaps: "both"are rewritten to[specifier]+sourceRoot: "".//# sourceMappingURL=comment would corrupt those payloads.The local-file branch (BRANCH B) and the
transformhook are intentionally left alone: real-file modules don't expose the virtual ID, and aggressively rewriting their maps produced doubled paths in test runs (the source-map composition with the loader's inline map ended up wrong).Tests
identitySourceMapandrewriteLoadedSourceMapinpackages/plugin-vite/src/plugins/deno_test.ts.vite dev - strips deno:: prefix from stack traces) and prod-build integration test (vite build - strips deno:: from server stack traces) that fetch/tests/throwand assert the response body contains neitherdeno::N::nor doubled cwd paths.Closes bartlomieju/orchid-inbox#56
Test plan
deno test -A packages/plugin-vite/src/plugins/deno_test.ts(6 unit tests pass)deno test -A packages/plugin-vite/tests/dev_server_test.ts(39 passed, 1 ignored)deno test -A packages/plugin-vite/tests/build_test.ts(34 passed)deno fmt --check,deno lint,deno task check:types