Uh oh!
There was an error while loading. Please reload this page.
fix(builders): improve step bundle discovery and externalization for SDK serde classes - #1669
Merged
Merged
Conversation
…SDK serde classes - Broaden importParents tracking to all imports (not just file extensions) so parentHasChild() works through bare specifier imports - Include workflow/runtime in discovery inputs so SDK serde classes like Run are always discovered - Bundle node_modules deps instead of externalizing with broken relative paths in CJS shims
🦋 Changeset detectedLatest commit: aebee64 The changes in this PR will be included in the next version bump. This PR includes changesets to release 16 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❌ Some tests failed Summary
❌ Failed Tests🌍 Community Worlds (65 failed)mongodb (4 failed):
redis (3 failed):
turso (58 failed):
Details by Category✅ ▲ Vercel Production
✅ 💻 Local Development
✅ 📦 Local Production
✅ 🐘 Local Postgres
✅ 🪟 Windows
❌ 🌍 Community Worlds
✅ 📋 Other
|
Contributor
📊 Benchmark Results
workflow with no steps💻 Local Development
▲ Production (Vercel)
🔍 Observability: Next.js (Turbopack) | Nitro workflow with 1 step💻 Local Development
▲ Production (Vercel)
🔍 Observability: Next.js (Turbopack) | Nitro workflow with 10 sequential steps💻 Local Development
▲ Production (Vercel)
🔍 Observability: Nitro | Next.js (Turbopack) workflow with 25 sequential steps💻 Local Development
▲ Production (Vercel)
🔍 Observability: Nitro | Next.js (Turbopack) workflow with 50 sequential steps💻 Local Development
▲ Production (Vercel)
🔍 Observability: Next.js (Turbopack) | Nitro Promise.all with 10 concurrent steps💻 Local Development
▲ Production (Vercel)
🔍 Observability: Next.js (Turbopack) | Nitro Promise.all with 25 concurrent steps💻 Local Development
▲ Production (Vercel)
🔍 Observability: Nitro | Next.js (Turbopack) Promise.all with 50 concurrent steps💻 Local Development
▲ Production (Vercel)
🔍 Observability: Nitro | Next.js (Turbopack) Promise.race with 10 concurrent steps💻 Local Development
▲ Production (Vercel)
🔍 Observability: Next.js (Turbopack) | Nitro Promise.race with 25 concurrent steps💻 Local Development
▲ Production (Vercel)
🔍 Observability: Nitro | Next.js (Turbopack) Promise.race with 50 concurrent steps💻 Local Development
▲ Production (Vercel)
🔍 Observability: Nitro | Next.js (Turbopack) workflow with 10 sequential data payload steps (10KB)💻 Local Development
▲ Production (Vercel)
🔍 Observability: Nitro | Next.js (Turbopack) workflow with 25 sequential data payload steps (10KB)💻 Local Development
▲ Production (Vercel)
🔍 Observability: Nitro | Next.js (Turbopack) workflow with 50 sequential data payload steps (10KB)💻 Local Development
▲ Production (Vercel)
🔍 Observability: Nitro | Next.js (Turbopack) workflow with 10 concurrent data payload steps (10KB)💻 Local Development
▲ Production (Vercel)
🔍 Observability: Nitro | Next.js (Turbopack) workflow with 25 concurrent data payload steps (10KB)💻 Local Development
▲ Production (Vercel)
🔍 Observability: Next.js (Turbopack) | Nitro workflow with 50 concurrent data payload steps (10KB)💻 Local Development
▲ Production (Vercel)
🔍 Observability: Nitro | Next.js (Turbopack) Stream Benchmarks(includes TTFB metrics)workflow with stream💻 Local Development
▲ Production (Vercel)
🔍 Observability: Nitro | Next.js (Turbopack) stream pipeline with 5 transform steps (1MB)💻 Local Development
▲ Production (Vercel)
🔍 Observability: Nitro | Next.js (Turbopack) 10 parallel streams (1MB each)💻 Local Development
▲ Production (Vercel)
🔍 Observability: Nitro | Next.js (Turbopack) fan-out fan-in 10 streams (1MB each)💻 Local Development
▲ Production (Vercel)
🔍 Observability: Next.js (Turbopack) | Nitro SummaryFastest Framework by WorldWinner determined by most benchmark wins
Fastest World by FrameworkWinner determined by most benchmark wins
Column Definitions
Worlds:
❌ Some benchmark jobs failed:
Check the workflow run for details. |
Contributor
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates @workflow/builders bundling/discovery behavior to better detect SDK-provided serde classes and avoid generating fragile externalized paths that break when downstream tools rebundle step bundles.
Changes:
- Track parent→child import relationships for all imports (including bare specifiers) so
parentHasChild()works through SDK entrypoints. - Add
workflow/runtimeto the discovery inputs so SDK serde classes (e.g.Run) are discovered even when not imported by user code. - Avoid externalizing resolved
node_modulesfiles via deep relative paths by letting esbuild bundle them instead.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| packages/builders/src/swc-esbuild-plugin.ts | Stops externalizing resolved node_modules files via relative paths; lets esbuild bundle them. |
| packages/builders/src/discover-entries-esbuild-plugin.ts | Broadens onResolve tracking to all imports to improve transitive parent/child detection through bare specifiers. |
| packages/builders/src/base-builder.ts | Adds workflow/runtime to discovery entrypoints to ensure SDK serde class discovery. |
| .changeset/builders-discovery-fixes.md | Patch changeset documenting the behavior fixes. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+428
to
+436
| const discoveryInputs = [...inputFiles]; | ||
| if (resolvedWorkflowRuntime) { | ||
| discoveryInputs.push(resolvedWorkflowRuntime); | ||
| } | ||
| const discovered = | ||
| discoveredEntries ?? | ||
| (await this.discoverEntries(inputFiles, dirname(outfile), tsconfigPath)); | ||
| (await this.discoverEntries( | ||
| discoveryInputs, | ||
| dirname(outfile), |
There was a problem hiding this comment.
discoveryInputs is always created as a new array ([...inputFiles]), which defeats discoverEntries() caching (it uses a WeakMap keyed by the inputs array reference). This can cause discovery to re-run on every call/watch rebuild even when inputFiles is unchanged. Consider passing inputFiles directly when no extra entry is needed, or memoizing a stable augmented array when adding resolvedWorkflowRuntime.
MemberAuthor
There was a problem hiding this comment.
Fixed. discoveryInputs now only creates a new array when resolvedWorkflowRuntime is present. When no extra entries are needed, inputFiles is passed directly to discoverEntries(), preserving the array reference for WeakMap caching.
Comment on lines
+82
to
90
| // Track parent→child import relationships for ALL imports (not just | ||
| // those with file extensions) so that `parentHasChild()` can correctly | ||
| // identify transitive parents of serde/step files even when the | ||
| // dependency chain passes through bare specifier imports like | ||
| // `@workflow/core/runtime` or `workflow/runtime`. | ||
| build.onResolve({ filter: /.*/ }, async (args) => { | ||
| try { | ||
| const resolved = await enhancedResolve(args.resolveDir, args.path); | ||
There was a problem hiding this comment.
The new onResolve({ filter: /.*/ }) behavior is intended to support bare specifier chains, but there’s no test asserting that importParents/parentHasChild() works through a bare specifier import (e.g. entry -> workflow/runtime -> serde file). Please add a vitest case that builds a small fixture with a bare specifier dependency and verifies parentHasChild() (or the downstream bundling decision) reflects that relationship.
MemberAuthor
There was a problem hiding this comment.
Added. New test tracks importParents through bare specifier imports sets up a fixture with entry.ts -> 'bare-pkg' -> ./serde.js and verifies:
importParentstracks both the bare specifier resolution (entry -> bare-pkg/index.js) and the relative import (index.js -> serde.js)parentHasChild(entry, serde)returnstruetransitively through the bare specifier
- Preserve inputFiles array reference for discoverEntries() WeakMap caching when no extra entries are needed - Add test verifying importParents/parentHasChild works through bare specifier imports (entry -> bare-pkg -> serde file)
VaguelySerious
left a comment
Member
There was a problem hiding this comment.
AI review: no blocking issues
| // identify transitive parents of serde/step files even when the | ||
| // dependency chain passes through bare specifier imports like | ||
| // `@workflow/core/runtime` or `workflow/runtime`. | ||
| build.onResolve({ filter: /.*/ }, async (args) => { |
There was a problem hiding this comment.
AI Review: Note
Using onResolve with filter: /.*/ means this hook runs for every resolved request during discovery. That tradeoff is reasonable for correctness, but it may increase discovery CPU on very large graphs. If this shows up in profiles later, consider whether a narrower filter plus a second pass is worth it.
MemberAuthor
There was a problem hiding this comment.
Good point. The discovery pass runs once (or once per watch rebuild) and the graph is bounded by the project's actual import tree, so this shouldn't be a bottleneck in practice. But agreed it's worth keeping an eye on for very large monorepos.
| // can't rewrite opaque `__require()` calls in CJS shims, so | ||
| // relative paths computed for `outdir` break once the output | ||
| // is rebundled to a different directory. | ||
| if (normalizedResolvedPath.includes('/node_modules/')) { |
There was a problem hiding this comment.
AI Review: Note
Bundling any resolved file under /node_modules/ avoids brittle relative externals for downstream rebundlers. That is likely the right default here; still worth watching step-bundle size regressions after this change, since it is broader than only fixing the broken externals case.
MemberAuthor
There was a problem hiding this comment.
Agreed. This is intentionally broad to fix the class of broken relative externals, but we should monitor step bundle sizes. In practice the node_modules files that hit this path are small CJS shims with internal require() calls (like @vercel/functions/headers.js), not large libraries.
| // traces through it and discovers serde classes (like `Run`) that | ||
| // live inside SDK packages. Without this, files like `run.js` are | ||
| // only discovered when user code happens to import them. | ||
| const resolvedWorkflowRuntime = await enhancedResolve( |
There was a problem hiding this comment.
AI Review: Nit
Silently skipping when workflow/runtime cannot be resolved preserves prior behavior, but it also means discovery still misses SDK-only serde files in misconfigured installs. If that scenario is worth surfacing, a debug log (or a more explicit warning mode) might help.
MemberAuthor
There was a problem hiding this comment.
Fair point. The silent catch is intentional to avoid breaking builds when workflow isn't installed yet (e.g. during initial setup), but a debug-level log would be helpful. Will add if this becomes a support issue.
VaguelySerious
approved these changes
Apr 9, 2026
TooTallNate
enabled auto-merge (squash)
April 9, 2026 19:28
Uh oh!
There was an error while loading. Please reload this page.
VaguelySerious added a commit
that referenced
this pull request
May 2, 2026
PR #1609 was authored on top of #1669 (`fix(builders): improve step bundle discovery and externalization for SDK serde classes`), which is on `main` but not on `stable`. Bring along just the bits #1609 depends on: - Broaden discover-entries onResolve filter from `jsTsRegex` to `/.*/` so the import graph captures bare and extensionless imports. - Switch the new test fixtures from `Set<string>` back to `string[]` to match stable's still-array-based state interface in `createDiscoverEntriesPlugin`.
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
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.
Summary
importParentstracking in the discovery plugin to all imports (not just file extensions) soparentHasChild()works through bare specifier imports like@workflow/core/runtimeworkflow/runtimein discovery inputs so SDK serde classes likeRunare always discovered even when no user code imports themnode_modulesdeps instead of externalizing with relative paths that break when downstream bundlers (Rollup/Vite) rebundle to a different directory