fix(builders): resolve path aliases when externalizing non-step imports#1613
Merged
Conversation
🦋 Changeset detectedLatest commit: 60ed9e5 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
Contributor
📊 Benchmark Results
workflow with no steps💻 Local Development
workflow with 1 step💻 Local Development
workflow with 10 sequential steps💻 Local Development
workflow with 25 sequential steps💻 Local Development
workflow with 50 sequential steps💻 Local Development
Promise.all with 10 concurrent steps💻 Local Development
Promise.all with 25 concurrent steps💻 Local Development
Promise.all with 50 concurrent steps💻 Local Development
Promise.race with 10 concurrent steps💻 Local Development
Promise.race with 25 concurrent steps💻 Local Development
Promise.race with 50 concurrent steps💻 Local Development
workflow with 10 sequential data payload steps (10KB)💻 Local Development
workflow with 25 sequential data payload steps (10KB)💻 Local Development
workflow with 50 sequential data payload steps (10KB)💻 Local Development
workflow with 10 concurrent data payload steps (10KB)💻 Local Development
workflow with 25 concurrent data payload steps (10KB)💻 Local Development
workflow with 50 concurrent data payload steps (10KB)💻 Local Development
Stream Benchmarks (includes TTFB metrics)workflow with stream💻 Local Development
stream pipeline with 5 transform steps (1MB)💻 Local Development
10 parallel streams (1MB each)💻 Local Development
fan-out fan-in 10 streams (1MB each)💻 Local Development
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❌ Some tests failed Summary
❌ Failed Tests🌍 Community Worlds (64 failed)mongodb (4 failed):
redis (3 failed):
turso (57 failed):
Details by Category✅ ▲ Vercel Production
✅ 💻 Local Development
✅ 📦 Local Production
✅ 🐘 Local Postgres
✅ 🪟 Windows
❌ 🌍 Community Worlds
✅ 📋 Other
|
matchai
changed the title
The isAbsolute(resolvedPath) condition for detecting file paths was too broad, matching bare npm specifiers (e.g. lodash.chunk) that enhanced-resolve already resolved to absolute node_modules paths. This caused Rollup to reject the relativized paths in Nitro builds. Gate the check behind resolvedViaEsbuild so it only applies to alias/tsconfig paths resolved by the esbuild fallback.
…larity Reject esbuild fallback results that resolve into node_modules to preserve the nested dep bundling invariant. Refactor resolution variables for clarity: separate specifier from resolvedPath, rename isFilePath to shouldMakeRelative, add regression test for aliased imports targeting node_modules.
Member
VaguelySerious
left a comment
VaguelySerious
left a comment
There was a problem hiding this comment.
AI review: no blocking issues
| const output = result.outputFiles[0].text; | ||
| expect(output).toContain('/lib/config.js'); | ||
| expect(output).not.toContain('@/lib/config'); | ||
| }); |
Member
There was a problem hiding this comment.
AI Review: Note
This test uses esbuild alias to set up the path alias, but the more common real-world scenario is tsconfig paths (which is how most Next.js / Vite projects configure @/* aliases). Consider adding a test that uses a tsconfig.json with compilerOptions.paths and passes it via esbuild's tsconfig option — this would exercise the same build.resolve() fallback path but through the tsconfig resolution pipeline that most users actually hit.
I verified locally that this does work (wrote a test using tsconfig: join(testRoot, 'tsconfig.json') with paths: { "@/*": ["./src/*"] } instead of alias), but having it in the test suite would increase confidence.
| didResolve && | ||
| !esbuildResult.path | ||
| .replace(/\\/g, '/') | ||
| .includes('/node_modules/'); |
Member
There was a problem hiding this comment.
AI Review: Nit
The /node_modules/ string check works for the common case but could theoretically match a project directory literally named node_modules in its path (e.g. /home/user/my-node_modules-tools/src/lib.ts). This is extremely unlikely and I don't think it warrants a change, but worth noting for future awareness.
| !esbuildResult.path | ||
| .replace(/\\/g, '/') | ||
| .includes('/node_modules/'); | ||
| if (isProjectLocalFile) { |
Member
There was a problem hiding this comment.
AI Review: Note
When the esbuild fallback resolves an aliased path that also appears in sideEffectEntries, the hasSideEffects check (line 170) uses normalizedResolvedPath — which at this point is correctly set from esbuildResult.path. So the sideEffects flag should propagate correctly for aliased imports. However, there's no test coverage for this interaction. A test where an aliased import matches a sideEffectEntries entry would be valuable.
VaguelySerious
approved these changes
Apr 6, 2026
ghost
mentioned this pull request
VaguelySerious
pushed a commit
that referenced
this pull request
Apr 7, 2026
VaguelySerious
added a commit
that referenced
this pull request
Apr 7, 2026
PR #1613 added an esbuild fallback for resolving path aliases when enhanced-resolve fails. However, Node.js builtins like `crypto` and `node:path` also fail enhanced-resolve, and esbuild resolves them to absolute paths outside node_modules. The `isProjectLocalFile` check then incorrectly relativizes them (e.g. `../../../../../../crypto`), breaking Next.js builds. Fix: check `isBuiltin(specifier)` before falling through to the esbuild resolver, so builtins are left as bare specifiers. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
3 tasks
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
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.
When step bundles externalize non-step imports that use path aliases (e.g.
@/lib/configvia tsconfigpathsor esbuildalias),enhanced-resolvecan't resolve them — it doesn't know about esbuild aliases or tsconfig path mappings. The unresolved import falls through to esbuild's native resolver which bundles it inline instead of externalizing.This causes
@workflow/vitestto fail at runtime withERR_MODULE_NOT_FOUNDfor any project using path aliases in step function dependencies, since the step bundle inlines the code instead of producing externalized imports that vite-node can intercept forvi.mock().Fix
When
enhanced-resolvefails for a non-relative import, fall back tobuild.resolve(esbuild's own resolver) which handles bothaliasconfig and tsconfigpaths. ApluginDataguard prevents infinite re-entry. Additionally,isAbsolute(resolvedPath)is checked when deciding whether to compute a relative external path, since resolved alias paths are absolute even though the original import specifier isn't.