Uh oh!
There was an error while loading. Please reload this page.
Fix Nitro dev loading of local step dependencies - #1965
Merged
Conversation
Contributor
🦋 Changeset detectedLatest commit: b3e11d8 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
📊 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: Next.js (Turbopack) | Express | Nitro workflow with 10 sequential steps💻 Local Development
▲ Production (Vercel)
🔍 Observability: Nitro | Next.js (Turbopack) | Express workflow with 25 sequential steps💻 Local Development
▲ Production (Vercel)
🔍 Observability: Nitro | Next.js (Turbopack) | Express workflow with 50 sequential steps💻 Local Development
▲ Production (Vercel)
🔍 Observability: Nitro | Express | Next.js (Turbopack) Promise.all with 10 concurrent steps💻 Local Development
▲ Production (Vercel)
🔍 Observability: Next.js (Turbopack) | Express | Nitro Promise.all with 25 concurrent steps💻 Local Development
▲ Production (Vercel)
🔍 Observability: Nitro | Next.js (Turbopack) | Express Promise.all with 50 concurrent steps💻 Local Development
▲ Production (Vercel)
🔍 Observability: Next.js (Turbopack) | Express | Nitro Promise.race with 10 concurrent steps💻 Local Development
▲ Production (Vercel)
🔍 Observability: Next.js (Turbopack) | Nitro | Express Promise.race with 25 concurrent steps💻 Local Development
▲ Production (Vercel)
🔍 Observability: Nitro | Next.js (Turbopack) | Express Promise.race with 50 concurrent steps💻 Local Development
▲ Production (Vercel)
🔍 Observability: Nitro | Next.js (Turbopack) | Express workflow with 10 sequential data payload steps (10KB)💻 Local Development
▲ Production (Vercel)
🔍 Observability: Next.js (Turbopack) | Express | Nitro workflow with 25 sequential data payload steps (10KB)💻 Local Development
▲ Production (Vercel)
🔍 Observability: Express | Next.js (Turbopack) | Nitro 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 | Next.js (Turbopack) | Nitro workflow with 25 concurrent data payload steps (10KB)💻 Local Development
▲ Production (Vercel)
🔍 Observability: Next.js (Turbopack) | Express | Nitro workflow with 50 concurrent data payload steps (10KB)💻 Local Development
▲ Production (Vercel)
🔍 Observability: Next.js (Turbopack) | Nitro | Express Stream Benchmarks(includes TTFB metrics)workflow with stream💻 Local Development
▲ Production (Vercel)
🔍 Observability: Nitro | Express | Next.js (Turbopack) stream pipeline with 5 transform steps (1MB)💻 Local Development
▲ Production (Vercel)
🔍 Observability: Nitro | Express | Next.js (Turbopack) 10 parallel streams (1MB each)💻 Local Development
▲ Production (Vercel)
🔍 Observability: Express | Nitro | 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:
|
Contributor
🧪 E2E Test Results✅ All tests passed Summary
Details by Category✅ ▲ Vercel Production
✅ 💻 Local Development
✅ 📦 Local Production
✅ 🐘 Local Postgres
✅ 🪟 Windows
✅ 📋 Other
|
ijjk
marked this pull request as ready for review
May 10, 2026 02:28
ijjk
enabled auto-merge (squash)
May 10, 2026 02:42
TooTallNate
approved these changes
May 11, 2026
TooTallNate
left a comment
Member
There was a problem hiding this comment.
Approve
Nice design — substantially better-scoped than #1609. The opt-in flag is the right call: changing externalization behavior unconditionally (as #1609 did) would have altered bundling for every downstream framework (Next/Turbopack, Next/Webpack, Vite, etc.), each of which already handles those externalized imports through their own bundler. This PR's approach keeps that path unchanged and only adds the new bundling behavior in the one environment that genuinely needs it (Nitro dev, where the generated .nitro/workflow/steps.mjs is loaded directly from disk by Node).
What I verified
enhanced-resolve config fix is correct. On main, enhanced-resolve is instantiated with no options, so its default extension set doesn't include .ts / .tsx / .mts / .cts. Extensionless imports like import './helpers' never resolve, importParents stays incomplete, and the later swc-plugin logic has no way to identify the file as a transitive local descendant. Adding the full TS extension list + fullySpecified: false fixes this for the discovery pass without affecting esbuild's own actual resolution (which already supports these via resolveExtensions).
isProjectLocalFile excludes the right things.
- Files inside
node_modules→ false (so plain package deps stay external even with the opt-in) - Files inside a sibling workspace package → false (via
resolveModuleSpecifier()returning a definedmoduleSpecifier, sinceisWorkspacePackagefinds a non-rootpackage.json) - Files in the project root tree with no nearer
package.json→ true (the local-app case)
That correctly captures the issue #1179 scenario (Nuxt app importing ~/shared/constants from project root) without accidentally bundling workspace-package files that should remain externalized.
Descendant check is correctly gated. The new parentHasChild(normalizedEntry, normalizedResolvedPath) is the inverse of the existing parentHasChild(normalizedResolvedPath, normalizedEntry) check — same importParents graph, opposite direction. Only runs when options.bundleTransitiveLocalStepDependencies is true, so existing behavior is byte-identical for non-Nitro-dev callers.
Wiring: LocalBuilder.createStepsBundle({ bundleTransitiveLocalStepDependencies: this.config.watch }) opts in when nitro.options.dev === true. Nuxt-via-@workflow/nuxt-via-@workflow/nitro gets the fix automatically (since #1179 was originally a Nuxt dev report, that's the important one).
Tests (packages/builders/src/swc-esbuild-plugin.test.ts):
bundles transitive local TypeScript dependencies with extensionless imports— the positive case from the bug reportexternalizes transitive local TypeScript dependencies by default— verifies the flag is genuinely opt-inkeeps ordinary package dependencies external when reachable from a bundled step— verifies node_modules stays external even with opt-inbundles package parents when they lead to discovered workflow-related entries— verifies the existingparentHasChild(resolved, entry)ancestor path still works
Good coverage — both the new behavior AND the back-compat default are explicitly asserted.
Build & tests: 161 / 161 @workflow/builders tests pass locally (up from 157 on main), workspace pnpm typecheck clean, 95 / 95 of the PR's CI checks green.
One small nit — see inline
Tiny issue with the changeset scope on .changeset/narrow-step-bundling.md — @workflow/nitro is also modified but not listed. Mechanically fine (auto-bumped via updateInternalDependencies), but AGENTS.md asks for explicit listing of all changed packages.
Otherwise: approving.
| '@workflow/builders': patch | ||
| --- | ||
| Bundle transitive local step dependencies for direct Nitro dev loading while keeping ordinary package dependencies external unless they contain workflow entries. |
Member
There was a problem hiding this comment.
Tiny nit on the changeset scope: @workflow/nitro is also modified (the bundleTransitiveLocalStepDependencies: this.config.watch flag in packages/nitro/src/builders.ts), but only @workflow/builders is listed here. Mechanically you're fine — @workflow/nitro depends on @workflow/builders and updateInternalDependencies: 'patch' in .changeset/config.json will auto-bump it. But AGENTS.md says "All changed packages should be included in the changeset", so the more conservative listing would be:
Suggested change
| Bundle transitive local step dependencies for direct Nitro dev loading while keeping ordinary package dependencies external unless they contain workflow entries. | |
| --- | |
| '@workflow/builders': patch | |
| '@workflow/nitro': patch | |
| --- | |
| Bundle transitive local step dependencies for direct Nitro dev loading while keeping ordinary package dependencies external unless they contain workflow entries. |
Non-blocking.
Uh oh!
There was an error while loading. Please reload this page.
github-actionsBot
added a commit
that referenced
this pull request
May 11, 2026
* Fix local step dependency bundling * Limit local dependency bundling to Nitro dev Signed-off-by: JJ Kasper <jj@jjsweb.site>
Contributor
Backport PR opened against |
TooTallNate added a commit
that referenced
this pull request
May 22, 2026
* Fix Nitro dev loading of local step dependencies (#1965) * Fix local step dependency bundling * Limit local dependency bundling to Nitro dev Signed-off-by: JJ Kasper <jj@jjsweb.site> * Fix stable backport build failures * Revert runtime workaround changes * Revert DOMException reducer change * Revert serialization test mock --------- Signed-off-by: JJ Kasper <jj@jjsweb.site> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Nathan Rajlich <n@n8.io> Co-authored-by: JJ Kasper <jj@jjsweb.site>
Merged
VaguelySerious added a commit
that referenced
this pull request
Jun 11, 2026
The vitest builder's generated bundles are imported directly by Node in the test worker, with no downstream bundler. Externalized project-local imports therefore surface as raw .ts specifiers that Node's native ESM loader cannot reliably load (only erasable syntax via type stripping, and not at all for extensionless transitive imports). Enable bundleTransitiveLocalStepDependencies (added for the same direct-loading situation in Nitro dev, #1965) so local helpers are inlined into the step bundle. Fixes#2289 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
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.
Summary
externalizeNonStepsexternalizing local deps for downstream bundlers like Nitro/RollupThis is a narrower alternative to #1609: instead of broadening the
externalizeNonStepsgraph for all downstream bundler paths, it only bundles transitive local step dependencies for Nitro dev's direct-from-disk loading path.Closes#1609
Closes#1179