Auto-remove workflow packages from serverExternalPackages#1481
Merged
TooTallNate merged 4 commits intomainfrom May 5, 2026
Merged
Auto-remove workflow packages from serverExternalPackages#1481TooTallNate merged 4 commits intomainfrom
TooTallNate merged 4 commits intomainfrom
Conversation
🦋 Changeset detectedLatest commit: aa85a83 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
Contributor
📊 Benchmark Results
workflow with no steps💻 Local Development
▲ Production (Vercel)
🔍 Observability: Nitro | Next.js (Turbopack) workflow with 1 step💻 Local Development
▲ Production (Vercel)
🔍 Observability: Nitro | Next.js (Turbopack) 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: Nitro | Next.js (Turbopack) Promise.all with 10 concurrent steps💻 Local Development
▲ Production (Vercel)
🔍 Observability: Nitro | Next.js (Turbopack) 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: Next.js (Turbopack) | Nitro Promise.race with 10 concurrent steps💻 Local Development
▲ Production (Vercel)
🔍 Observability: Nitro | Next.js (Turbopack) 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: Next.js (Turbopack) | Nitro 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: Nitro | Next.js (Turbopack) 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
🧪 E2E Test Results✅ All tests passed Summary
Details by Category✅ ▲ Vercel Production
✅ 💻 Local Development
✅ 📦 Local Production
✅ 🐘 Local Postgres
✅ 🪟 Windows
✅ 📋 Other
|
Contributor
There was a problem hiding this comment.
Pull request overview
Adds a best-effort build-time warning in the builders layer when a user externalizes a package that appears to contain workflow constructs (step/workflow directives or serialization patterns), and documents the “externalization footgun” in the serialization guide.
Changes:
- Add
BaseBuilder.warnAboutExternalWorkflowPackages()and invoke it after the discovery phase. - Add Vitest coverage for the warning behavior across multiple detection paths and edge cases.
- Document third-party package externalization pitfalls in the serialization guide, plus a changeset entry.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| packages/builders/src/base-builder.ts | Implements external package workflow-pattern detection and emits warnings after discovery. |
| packages/builders/src/external-package-warning.test.ts | Adds tests verifying warning emission and suppression behavior. |
| docs/content/docs/foundations/serialization.mdx | Documents that workflow-exporting packages must not be externalized (e.g., Next.js serverExternalPackages). |
| .changeset/warn-external-workflow-packages.md | Publishes a patch note for the new warning behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
VaguelySerious
previously approved these changes
Mar 23, 2026
pranaygp
reviewed
Mar 23, 2026
Contributor
pranaygp
left a comment
pranaygp
left a comment
There was a problem hiding this comment.
Overall this is a well-scoped, useful addition. The detection logic is sound, tests are comprehensive, and the docs addition is clear. One real bug to fix (require.resolve in ESM) and a couple of minor suggestions.
Contributor
karthikscale3
left a comment
karthikscale3
left a comment
There was a problem hiding this comment.
Here's my review of PR #1481: Warn when serverExternalPackages hides workflow-enabled packages.
Summary
This PR adds a build-time warning when packages listed in serverExternalPackages (or externalPackages) contain workflow code that would be invisible to the workflow compiler. It also adds documentation in the serialization guide explaining the externalization footgun. The problem is well-motivated -- externalized packages silently fail at runtime because the SWC transform never runs on them.
Blocker: require.resolve in ESM context
Both Copilot and @pranaygp already flagged this, and I agree -- this is a real bug that needs fixing before merge.
@workflow/builders is "type": "module", so bare require.resolve() will throw ReferenceError: require is not defined at runtime. The tests pass because Vitest transpiles to CJS, masking the issue. Both require.resolve() calls (lines 188 and 207) are wrapped in try/catch, so the warning logic will silently never run in production.
The fix is straightforward -- the codebase already has the pattern in apply-swc-transform.ts:
import { createRequire } from 'node:module';
const require = createRequire(import.meta.url);Add the same to base-builder.ts (or scope it inside the method).
No Other Blockers
Beyond the ESM bug, the implementation is solid.
Code Review Details
Detection logic is well-designed
The two-tier approach is smart:
- Fast path: Check
package.jsonfor@workflow/serdein deps/peerDeps - Thorough path: Read the entry file and run
detectWorkflowPatterns()(reusing existing regex-based detection fromtransform-utils.ts)
The triple try/catch nesting (outer best-effort, inner fast-path, inner thorough-path) ensures the warning never blocks a build regardless of what goes wrong.
Warning message is actionable
The warning clearly identifies: which package, what was found (step functions, workflow functions, serialization classes), and exactly how to fix it (Remove "pkg" from serverExternalPackages).
Minor issues flagged in existing reviews (agree with all)
-
warnedExternalPackages.add(pkg)placement -- The package is added to the dedup set before checking if a warning fires. If detection fails (e.g., partial install), the package is permanently skipped for this builder instance. Since builders are short-lived, this is low-impact, but moving.add(pkg)to just beforeconsole.warn()would be more correct. -
Hard-coded
serverExternalPackagesin warning text -- The config field isexternalPackages. Non-Next.js users (Astro, SvelteKit, etc.) may be confused by the Next.js-specific name. @pranaygp's suggestion to sayexternalPackages (serverExternalPackages in Next.js)is good. -
Entry-point-only detection --
require.resolve(pkg)only reads the main entry file. If workflow constructs are in sub-paths (e.g.,my-pkg/workflows), regex detection misses them. The@workflow/serdedep check partially covers serde cases. This is acceptable as a best-effort heuristic -- worth a code comment.
Tests are comprehensive (337 lines, 9 tests)
Covers: serde dep detection, serde symbol detection, "use step", "use workflow", combined patterns, pseudo-package skipping, clean packages, dedup behavior, and @workflow/serde in peerDependencies. One gap noted by @pranaygp: no test where package.json resolution fails but entry-file detection succeeds (testing that fast-path failure doesn't block thorough-path).
Documentation addition is clear
The new "Third-Party Packages" section in docs/content/docs/foundations/serialization.mdx explains the problem, shows the warning message, and provides the fix. Good placement.
Changeset is correct
Only @workflow/builders is listed as patch -- appropriate since the warning is contained within that package.
CI Status
4 failures, none related:
- Benchmark Vercel (express), Benchmark Local (nitro-v3), Benchmark Community World (MongoDB) -- infra timeouts (1h+)
- Vercel workbench-nestjs-workflow deployment -- pre-existing NestJS deployment issue (see PR #1485)
All E2E tests pass (780 Vercel prod, 782 local dev/prod/postgres, 72 Windows). Community world failures are the usual turso/redis/mongodb pre-existing issues.
Verdict: Fix the require.resolve ESM bug (use createRequire), then ship it. The minor suggestions (dedup timing, warning text, code comment) are nice-to-haves.
pranaygp
reviewed
Mar 27, 2026
pranaygp
pushed a commit
to vercel/sandbox
that referenced
this pull request
Mar 27, 2026
…ibility (#109) ## Summary Makes `@vercel/sandbox` fully compatible with the Workflow DevKit compiler so that `Sandbox`, `Command`, and `CommandFinished` instances can be used directly inside `"use workflow"` functions — no wrapper step functions needed. Supersedes #58. ## Problem When `@vercel/sandbox` is imported in a workflow context, the workflow builder tries to bundle it into the workflow VM bundle (because it has `WORKFLOW_SERIALIZE`/`WORKFLOW_DESERIALIZE` on its classes). This fails because: 1. The SDK's public methods use Node.js APIs (`fs`, `stream`, `zlib`, `undici`, etc.) which are forbidden in the workflow VM 2. The compiled `dist/index.js` was a single bundled file that hoisted all Node.js imports to the top, making them impossible to tree-shake 3. The `Sandbox` and `Command` classes had a sync `client` getter that directly referenced `APIClient`, pulling the entire HTTP client stack into the module scope ## Solution ### 1. `"use step"` annotations on all public async methods Added `"use step"` to all 22 public async methods across `Sandbox` (14), `Command` (5), and `Snapshot` (3). The SWC plugin strips these method bodies in workflow mode, replacing them with durable step proxies. This eliminates all Node.js API references from the workflow bundle. ### 2. Async `ensureClient()` replaces sync `client` getter The sync `get client()` getter directly referenced `APIClient`, which pulls in `undici`, `zlib`, `tar-stream`, `jsonlines`, etc. Replaced with: ```typescript private async ensureClient(): Promise<APIClient> { "use step"; if (this._client) return this._client; const credentials = getSandboxCredentials(); this._client = new APIClient({ ... }); return this._client; } ``` Since `ensureClient()` is itself `"use step"`, its body (including `new APIClient(...)`) gets stripped in workflow mode. All instance methods now call `const client = await this.ensureClient();` instead of `this.client`. ### 3. `bundle: false` in tsdown config Changed from single-file bundling to per-file output. This keeps Node.js imports local to the files that use them, so after the SWC plugin strips step method bodies, the now-unused Node.js imports can be eliminated by esbuild's tree-shaking. ### 4. Workflow-code-runner example updated - Removed `serverExternalPackages: ["@vercel/sandbox"]` from `next.config.ts` (no longer needed) - Updated `workflow` dependency to use a tarball that includes the inline class serialization registration fix (vercel/workflow#1480) ## Testing - `pnpm build` succeeds for the full monorepo (all 8 tasks) - The `workflow-code-runner` example app builds successfully with all workflow routes generated - 22 `"use step"` directives survive compilation in both ESM and CJS dist output ## Related - vercel/workflow#1480 — Inline class serialization registration (fixes SWC import resolution for 3rd-party packages) - vercel/workflow#1481 — Build-time warning when `serverExternalPackages` hides workflow-enabled packages - vercel/workflow#1144 — CJS detection for serde symbols (pending review) - #58 — Previous attempt (superseded by this PR) --------- Signed-off-by: Peter Wielander <mittgfu@gmail.com> Co-authored-by: Peter Wielander <mittgfu@gmail.com>
VaguelySerious
approved these changes
Apr 28, 2026
ijjk
approved these changes
Apr 29, 2026
Add a build-time warning when packages in serverExternalPackages contain
workflow code ('use step', 'use workflow', or serialization classes).
These packages are completely invisible to the workflow compiler when
externalized, causing silent runtime failures.
The warning detects workflow patterns via two methods:
- Fast path: check package.json dependencies for @workflow/serde
- Thorough path: read the package entry file and run pattern detection
Also adds documentation in the serialization guide about the
externalization footgun for 3rd-party packages.
When workflow-enabled dependencies are externalized in Next.js, compiler transforms are skipped and runtime failures follow. Detect those packages in withWorkflow, remove them from serverExternalPackages for the current build, and keep a generalized externalPackages warning fallback for non-Next builders.
Contributor
|
Backport PR opened against |
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 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.
Summary
nextConfig.serverExternalPackages(\"use step\",\"use workflow\", or serialization patterns) and auto-remove them for the current build so Next.js can transform them.externalPackages, and update warning text to useexternalPackageswith a Next.js mapping note.Why
Warning-only behavior still leaves users on a failure path at runtime. Auto-removing detected workflow packages from
serverExternalPackagesprovides a safer default UX while still giving users a clear warning and manual cleanup path innext.config.Implementation details
withWorkflow()now scans configuredserverExternalPackages, filters out detected workflow packages, and passes the filtered list to both Next config and builder externals.serverExternalPackagesto silence future warnings.createRequire(import.meta.url)Testing
pnpm vitest run packages/next/src/index.test.ts packages/builders/src/external-package-warning.test.tspnpm build(inpackages/next)pnpm build(inpackages/builders)