Repository navigation
fix(build): keep dynamic third-party imports bare in published dist - #2395
Conversation
The published `dist/shims/link.js` baked an absolute build-machine path into
the lazy Pages Router import:
loadRouter: async () =>
(await import("/<build-host>/node_modules/.pnpm/next@.../next/router.js"))
That path only exists on the machine that built vinext, so every consumer (and
CI) build fails with:
[UNRESOLVED_IMPORT] Could not resolve '/.../next/router.js'
Root cause: cloudflare#2279 replaced `deps.skipNodeModulesBundle: true` with a
`deps.neverBundle` predicate that keys off the *resolved* id
(`id.includes("node_modules")`). Rolldown resolves a dynamic `import()` literal
to an absolute path *before* `neverBundle` runs, so the bare specifier is lost
for dynamic imports (static imports were unaffected).
Restore the pre-cloudflare#2279 behaviour by externalizing on the *unresolved* bare
specifier via `inputOptions.external`, so dynamic imports stay bare like static
ones. First-party `vinext`/`@vinext/*` self-imports and the `alwaysBundle`
packages (`am-i-vibing`, `process-ancestry`) are excluded so they keep being
bundled/relative-rewritten.
Verified the emitted `dist` is byte-identical to the current build except the
single fixed `next/router` line, and the `am-i-vibing` bundle is unchanged.
|
/bigbonk review for issues |
commit: |
Performance benchmarksCompared 0 improved · 1 regressed · 5 within ±1.5%
View detailed results and traces 🟢 improvement · 🔴 regression · ⚫ change below 1.5% · paired base/head |
|
@james-elicx Bonk workflow was cancelled. View workflow run · To retry, trigger Bonk again. |
|
/bigbonk review for issues |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: fix(build): keep dynamic third-party imports bare in published dist
The core fix is correct. Switching externalization to the unresolved bare specifier via inputOptions.external restores the pre-#2279 behaviour, so dynamic import() literals stay bare like static imports instead of baking a build-machine absolute path into dist. I verified the diagnosis against the committed local build:
packages/vinext/dist/shims/link.js:585does indeed containimport("/home/runner/work/vinext/vinext/node_modules/.pnpm/next@.../next/router.js")— the exact baked path the PR describes.distis gitignored (not tracked), so changing onlyvite.config.tsand letting CI/release rebuild is the right scope.
The exclusion set is correct for the current package layout:
am-i-vibing/process-ancestryboth expose only a.export (no subpaths), so exact-match exclusion is sufficient.- The bundled
am-i-vibingimportsprocess-ancestry— handled both as a bare specifier (id === "process-ancestry") and as the resolved relative path (id.startsWith(".")). @vinext/cloudflare/cache/*subpath imports (the only real first-party externals in source) matchisFirstParty, so the alias-to-source bundling path is preserved.
Two things I'd want addressed before merge, plus one minor note. None block the correctness of the fix, but the first is what would have prevented this regression in the first place.
1. No regression test guards the published dist (most important)
This exact bug shipped via #2279 because nothing asserts that the emitted dist is free of baked absolute node_modules paths. There is no build/pack test that inspects the published output for this hazard (I checked tests/*build*.test.ts, clean-build-output.test.ts, server-external-packages.test.ts, and scripts/). Without a guard, the next refactor of external/neverBundle/alwaysBundle can silently reintroduce it. Per AGENTS.md ("Add tests first", dev/prod parity), please add a focused test that builds the package (or scans dist after vp pack) and asserts zero import("/...node_modules...") absolute specifiers remain — ideally covering a dynamic import in a shim like link.js.
2. Exclusion list duplicates alwaysBundle and must be kept in sync manually
The am-i-vibing / process-ancestry set now lives in three places (alwaysBundle, the neverBundle !id.includes(...) checks, and the external predicate). Adding/removing an alwaysBundle package requires remembering to update all three, or the new package silently gets externalized (the very failure mode this PR fixes). Deriving the exclusion from a single shared constant would make this self-consistent.
3. Minor: id.includes(":") is broader than the intent
The comment says this targets virtual/protocol-prefixed ids (node:, virtual:, \0...). includes(":") rather than a prefix check is slightly loose — it would also match any bare specifier that happens to contain a colon. No npm package name can contain :, so this is safe in practice today, but a prefix-oriented check (or a comment noting the intent) would be more precise and future-proof.
| if (isFirstParty(id)) return false; | ||
| // Packages inlined into `dist` via `alwaysBundle` must keep resolving so they | ||
| // get bundled rather than externalized. | ||
| if (id === "am-i-vibing" || id === "process-ancestry") return false; |
There was a problem hiding this comment.
The am-i-vibing / process-ancestry set is now duplicated across three locations: alwaysBundle (line 71), the neverBundle predicate (lines 74-75), and this external predicate. These must stay in sync — if a future alwaysBundle package isn't also added here, it gets externalized instead of inlined, silently reintroducing the class of bug this PR fixes.
Consider hoisting a single shared constant (e.g. const ALWAYS_BUNDLE = ["am-i-vibing", "process-ancestry"]) and reusing it in all three places — here as if (ALWAYS_BUNDLE.includes(id)) return false;, in neverBundle as !ALWAYS_BUNDLE.some((p) => id.includes(p)), and as the alwaysBundle value itself.
Also note this relies on both packages exposing only a . export (verified — no subpaths), so exact === matching is sufficient today. A new alwaysBundle package with subpath exports would need a prefix-aware check instead.
| if (isResolved) return false; | ||
| // Relative/absolute paths, virtual modules, and protocol-prefixed ids | ||
| // resolve normally. | ||
| if (id.startsWith(".") || id.startsWith("/") || id.startsWith("\0") || id.includes(":")) { |
There was a problem hiding this comment.
id.includes(":") is broader than the stated intent (matching node: / virtual: / protocol-prefixed ids). It matches a colon anywhere in the specifier. No valid npm package name contains :, so this is safe today, but a prefix-oriented check would more precisely express the intent and avoid surprises if a future resolver hands you an id with an embedded colon. Optional, but worth a clarifying note at minimum.
|
Review posted successfully. Here's a summary of my findings: Review summary for PR #2395Verdict: the core fix is correct. Switching to externalize on the unresolved bare specifier via Verified:
Findings posted:
None of these block correctness; the test gap is the only one I'd consider a should-fix. |
Problem
Consumer (and CI) builds against the published
vinextfail with:The published
dist/shims/link.jsbaked an absolute build-machine path into the lazy Pages Router import:That path only exists on the machine that built vinext, so it can never resolve on a consumer's machine.
Root cause
#2279 replaced
deps.skipNodeModulesBundle: truewith adeps.neverBundlepredicate that decides externalization on the resolved id (id.includes("node_modules")).Rolldown resolves a dynamic
import()literal to an absolute path beforeneverBundleruns, so the bare specifier is lost for dynamic imports. Static imports (e.g.import Router from "next/router"indev-server.js) were unaffected and stayed bare — which is why only the dynamicimport("next/router")inlink.jsbroke.skipNodeModulesBundle: truepreviously externalized on the unresolved bare specifier, so dynamic imports stayed bare.Fix
Externalize on the unresolved bare specifier via
inputOptions.external, restoring the pre-#2279 behaviour so dynamic imports stay bare like static ones. Excluded from forced-externalization (so they keep resolving/bundling as before):vinext/@vinext/*self-imports — must stay relative/bundled so shim modules are a single instance across Vite's RSC/SSR/client graphs (theinstanceof ReadonlyURLSearchParamshazard the oldskipNodeModulesBundlecomment called out).alwaysBundlepackagesam-i-vibing/process-ancestry— must stay inlined.Verification
distis byte-identical to the current build except the single fixed line:import("/abs/.../next/router.js")→import("next/router").am-i-vibingdependency tree (am-i-vibing→process-ancestry, no further deps) is still inlined underdist/node_modules/and is checksum-identical to the current build.node_modulesdynamic imports remain indist.