Skip to content

fix(build): don't path-resolve bare specifier aliases (#1507)#1843

Merged
james-elicx merged 1 commit into
mainfrom
fix/issue-1507-esm-externals
Jun 8, 2026
Merged

fix(build): don't path-resolve bare specifier aliases (#1507)#1843
james-elicx merged 1 commit into
mainfrom
fix/issue-1507-esm-externals

Conversation

@james-elicx

Copy link
Copy Markdown
Member

Fixes #1507experimental.esmExternals builds failed with No such file or directory because normalizeAliasEntries ran path.resolve on every alias value, turning bare package specifiers into bogus absolute paths. Now only .//..//absolute values are resolved; bare specifiers pass through verbatim (matching the existing index.ts:1250 convention). Tests added; all 174 pre-existing config tests pass. Closes #1507

@james-elicx

Copy link
Copy Markdown
Member Author

/bigbonk review for issues

@pkg-pr-new

pkg-pr-new Bot commented Jun 8, 2026

Copy link
Copy Markdown

Open in StackBlitz

npm i https://pkg.pr.new/@vinext/cloudflare@1843
npm i https://pkg.pr.new/vinext@1843

commit: aef86e0

@github-actions

github-actions Bot commented Jun 8, 2026

Copy link
Copy Markdown
Contributor
Example Preview Production Original
app-router-cloudflare preview production
pages-router-cloudflare preview production
app-router-playground preview production original
realworld-api-rest preview production
nextra-docs-template preview production
benchmarks preview production
hackernews preview production original
workers-cache preview production
web preview production

@james-elicx

Copy link
Copy Markdown
Member Author

/bigbonk review for issues

@ask-bonk

ask-bonk Bot commented Jun 8, 2026

Copy link
Copy Markdown
Contributor

@james-elicx Bonk workflow was cancelled.

View workflow run · To retry, trigger Bonk again.

@ask-bonk

ask-bonk Bot commented Jun 8, 2026

Copy link
Copy Markdown
Contributor

/bigbonk review — PR #1843

Verdict: LGTM. The fix is correct, minimal, and well-tested.

What the change does

normalizeAliasEntries previously ran path.resolve(root, value) on every non-absolute alias value, mangling bare package specifiers (react, preact/compat, @scope/pkg) into bogus <root>/react paths. The new isRelativeAliasTarget gate resolves only ., .., ./*, ../*, and absolute values; bare specifiers now pass through verbatim to Vite's resolve.alias, which re-resolves them via node_modules. This is the right fix for #1507.

Verification

  • All three callers of normalizeAliasEntries (turbopack resolveAlias ×2 + webpack resolve.alias) are alias maps where bare specifiers are valid Next.js input, so the relaxed resolution is correct for each — no caller relied on always-filesystem-resolving.
  • Aliases flow into resolve.alias at index.ts:1854, so passing bare specifiers through is exactly what Vite expects.
  • The new gate is slightly more robust than the cited index.ts:1250 convention (it also handles bare ./.., which the instrumentation-inject path does not).
  • tests/next-config.test.ts -t "alias" → 24 passed; tests/resolve-alias-build.test.ts → 1 passed. The build-driven regression test reproduces the real symptom end-to-end (imports an uninstalled preact/compat resolved purely via alias).

Minor / non-blocking observations

  • Windows backslash relative paths (.\foo, ..\foo) are not detected by startsWith("./")/startsWith("../") and would now fall through to the bare-specifier branch instead of being resolved. This matches the pre-existing index.ts:1250 convention and the old code only differed by always calling path.resolve, so it's not a regression introduced here — but worth a follow-up if Windows configs surface it.
  • webpack resolve.alias non-string values (arrays, or false to null out a module) are still silently dropped by the typeof value !== "string" guard. Pre-existing, out of scope for this PR.

No correctness or dev/prod-parity issues found. Good test coverage, clear JSDoc linking back to #1507.

@ask-bonk

ask-bonk Bot commented Jun 8, 2026

Copy link
Copy Markdown
Contributor

Review posted to PR #1843.

Summary

Verdict: LGTM — the fix is correct, minimal, and well-tested.

The change fixes #1507 cleanly. normalizeAliasEntries was unconditionally running path.resolve(root, value) on non-absolute alias values, which mangled bare package specifiers (react, preact/compat) into bogus <root>/react filesystem paths and broke esmExternals builds. The new isRelativeAliasTarget gate resolves only ./.././*/../*/absolute values and passes bare specifiers through verbatim so Vite resolves them via node_modules.

Verification I did:

  • Confirmed all 3 callers of normalizeAliasEntries are alias maps where bare specifiers are valid Next.js input — no caller depended on the old always-resolve behavior.
  • Confirmed aliases flow into resolve.alias at index.ts:1854, so passing bare specifiers through is exactly what Vite expects.
  • Ran the tests: tests/next-config.test.ts alias suite (24 passed) and the new tests/resolve-alias-build.test.ts build-driven regression (1 passed).

Non-blocking notes flagged in the review: Windows backslash relative paths and non-string webpack alias values are both pre-existing limitations, not regressions introduced by this PR.

github run

@james-elicx james-elicx marked this pull request as ready for review June 8, 2026 13:57
@james-elicx james-elicx merged commit 89a12bb into main Jun 8, 2026
51 checks passed
@james-elicx james-elicx deleted the fix/issue-1507-esm-externals branch June 8, 2026 18:22
@github-actions github-actions Bot mentioned this pull request Jun 8, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Build failure: esmExternals config breaks production build

1 participant