Uh oh!
There was an error while loading. Please reload this page.
Allow disabling step sourcemap with new sourcemap option in builders - #1842
Merged
Conversation
Signed-off-by: Peter Wielander <mittgfu@gmail.com>
Contributor
🦋 Changeset detectedLatest commit: 7065f19 The changes in this PR will be included in the next version bump. This PR includes changesets to release 18 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Contributor
🧪 E2E Test Results✅ All tests passed Summary
Details by Category✅ ▲ Vercel Production
✅ 💻 Local Development
✅ 📦 Local Production
✅ 🐘 Local Postgres
✅ 🪟 Windows
✅ 📋 Other
|
Contributor
📊 Benchmark Results
workflow with no steps💻 Local Development
▲ Production (Vercel)
🔍 Observability: Express | Nitro | Next.js (Turbopack) workflow with 1 step💻 Local Development
▲ Production (Vercel)
🔍 Observability: Nitro | Express | Next.js (Turbopack) workflow with 10 sequential steps💻 Local Development
▲ Production (Vercel)
🔍 Observability: Nitro | Express | Next.js (Turbopack) workflow with 25 sequential steps💻 Local Development
▲ Production (Vercel)
🔍 Observability: Nitro | Express | Next.js (Turbopack) workflow with 50 sequential steps💻 Local Development
▲ Production (Vercel)
🔍 Observability: Express | Next.js (Turbopack) | Nitro Promise.all with 10 concurrent steps💻 Local Development
▲ Production (Vercel)
🔍 Observability: Nitro | Next.js (Turbopack) | Express Promise.all with 25 concurrent steps💻 Local Development
▲ Production (Vercel)
🔍 Observability: Nitro | Express | Next.js (Turbopack) Promise.all with 50 concurrent steps💻 Local Development
▲ Production (Vercel)
🔍 Observability: Nitro | Express | Next.js (Turbopack) Promise.race with 10 concurrent steps💻 Local Development
▲ Production (Vercel)
🔍 Observability: Nitro | Express | Next.js (Turbopack) Promise.race with 25 concurrent steps💻 Local Development
▲ Production (Vercel)
🔍 Observability: Express | Nitro | Next.js (Turbopack) Promise.race with 50 concurrent steps💻 Local Development
▲ Production (Vercel)
🔍 Observability: Express | Next.js (Turbopack) | Nitro workflow with 10 sequential data payload steps (10KB)💻 Local Development
▲ Production (Vercel)
🔍 Observability: Express | Nitro | Next.js (Turbopack) workflow with 25 sequential data payload steps (10KB)💻 Local Development
▲ Production (Vercel)
🔍 Observability: Nitro | Express | Next.js (Turbopack) workflow with 50 sequential data payload steps (10KB)💻 Local Development
▲ Production (Vercel)
🔍 Observability: Express | Nitro | Next.js (Turbopack) workflow with 10 concurrent data payload steps (10KB)💻 Local Development
▲ Production (Vercel)
🔍 Observability: Express | Nitro | Next.js (Turbopack) workflow with 25 concurrent data payload steps (10KB)💻 Local Development
▲ Production (Vercel)
🔍 Observability: Express | Next.js (Turbopack) | Nitro workflow with 50 concurrent data payload steps (10KB)💻 Local Development
▲ Production (Vercel)
🔍 Observability: Nitro | Express | Next.js (Turbopack) Stream Benchmarks(includes TTFB metrics)workflow with stream💻 Local Development
▲ Production (Vercel)
🔍 Observability: Express | Nitro | Next.js (Turbopack) stream pipeline with 5 transform steps (1MB)💻 Local Development
▲ Production (Vercel)
🔍 Observability: Express | Nitro | Next.js (Turbopack) 10 parallel streams (1MB each)💻 Local Development
▲ Production (Vercel)
🔍 Observability: Nitro | Express | Next.js (Turbopack) fan-out fan-in 10 streams (1MB each)💻 Local Development
▲ Production (Vercel)
🔍 Observability: Nitro | Express | Next.js (Turbopack) SummaryFastest Framework by WorldWinner determined by most benchmark wins
Fastest World by FrameworkWinner determined by most benchmark wins
Column Definitions
Worlds:
Check the workflow run for details. |
TooTallNate
commented
Apr 23, 2026
Member
(Coming over from #1843, which I just closed as a duplicate — this PR is more comprehensive, particularly the framework-wide coverage and docs updates.) A few suggestions that came out of that branch, in case they're useful: 1. Consider using esbuild's native sourcemap vocabularyInstead of
Concretely the type would be: exporttypeSourcemapMode=boolean|'inline'|'linked'|'external'|'both';2. Add a |
Replace the bespoke `boolean | 'inline' | 'disabled'` type with esbuild's full `SourcemapMode` vocabulary (adds `'linked'`, `'external'`, `'both'`) so users can ship sourcemaps to observability tooling without bloating the function bundle. Add a `WORKFLOW_SOURCEMAP` env var as a cross-framework override; config wins over env, env wins over the per-bundle default. Flip `shouldAddSourcemapSupport` to track `sourcemapsEnabled` on the Vercel step function so disabling sourcemaps also drops the runtime shim. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
VaguelySerious
marked this pull request as ready for review
April 23, 2026 21:47
Signed-off-by: Peter Wielander <mittgfu@gmail.com>
TooTallNate
approved these changes
May 4, 2026
TooTallNate
left a comment
Member
There was a problem hiding this comment.
Verified the implementation end-to-end. The shape is right and the precedence rules are well-tested.
What's right
- Type vocabulary matches esbuild (
boolean | 'inline' | 'linked' | 'external' | 'both'). Users can pass through whatever esbuild accepts without the SDK getting in the way. - Precedence is correct and tested: explicit
config.sourcemap>WORKFLOW_SOURCEMAPenv > per-call-site default. The 10 new tests inresolve-sourcemap.test.tscover every branch including the env-var aliases (0/1/true/false), unrecognized values (warn + fall through), and thesourcemapsEnabledderived getter. All 145 builders tests pass on the branch. sourcemapsEnabledis the right primitive for downstream decisions like skipping the source-map-support runtime shim (vercel-build-output-api.ts:86). That shim isn't trivial in size, so dropping it forsourcemap: falseis a meaningful chunk of the 250MB savings.- Backwards compat preserved: the legacy
WORKFLOW_EMIT_SOURCEMAPS_FOR_DEBUGGING=1keeps working as the per-call-site default for the wrapper/webhook bundles. The "minor semantics change" the changeset flags (explicitsourcemap: ...now overrides the legacy env for those bundles too) is the right resolution — it's the natural meaning of "explicit > env > default" — and the changeset is upfront about it. - Discover-phase
sourcemap: falseat line 295 is left untouched. That's correct —write: falsemeans the discover pass doesn't emit anything, sourcemaps would be wasted compute. - Plumbing is consistent across all five framework adapters (Astro, NestJS, Next.js, Nitro, SvelteKit) — same option name, same type, same docs blurb pattern. Nuxt flows through to Nitro transitively (no separate plumbing needed since Nuxt is already a thin wrapper), so its absence from the changeset is correct.
- CLI works via env var route —
WORKFLOW_SOURCEMAP=false workflow buildworks without any CLI plumbing because all builders inheritBaseBuilder.resolveSourcemap. So the CLI/express/fastify/hono getting-started paths can use this without an additional config layer. - Documentation is comprehensive: every framework getting-started page that shows config gets a sourcemap row, plus a dedicated "Source maps" section in
with-workflow.mdxcovering all the modes, the size-vs-stack-trace tradeoff, and the env var precedence.
Nits (non-blocking)
- Type duplication.
SourcemapModeis exported from@workflow/builders/types.tsbut then redefined inline in 6 places (the framework adapter types and a couple of doc strings). All five framework packages already depend on@workflow/builders, so they could justimport type { SourcemapMode }and reuse. If the union ever grows (e.g. esbuild adds a new mode), all 6 sites have to be updated in lockstep. Trivial cleanup, would also DRY up the doc comments which are nearly identical. @workflow/nuxtgetting-started docs don't mentionsourcemapeven though it flows through to Nitro and works. Thenuxt.mdxpage doesn't have a config-options table at all currently, so this isn't a regression — but consider adding parallel coverage for completeness.parseSourcemapEnvwarning writes directly toconsole.warn. Most of the codebase uses the structuredruntimeLoggerfor this kind of thing. Probably fine for build-time warnings but worth aligning if there's a builder-side logger.
Tested
pnpm vitest run packages/builders/src/resolve-sourcemap.test.ts→ 10/10 pass.pnpm testinpackages/builders→ 145/145 pass.
LGTM.
This was referenced May 4, 2026
This was referenced May 5, 2026
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 freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
No description provided.