fix(build): preserve transitive external versions - #2887
Conversation
|
/bigbonk review for issues |
commit: |
|
|
/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: transitive external version preservation
Reviewed the three files. The overall approach is sound and faithfully mirrors Next.js baseResolveCheck (resolve from importer, resolve from root, bundle when they differ). The new E2E test is a good port of the Next.js suite and passes locally. A few findings below, ordered by severity.
1. fs.realpathSync output is used as a Vite module id without toSlash — Windows id-invariant violation (medium)
packages/vinext/src/plugins/transitive-externals.ts:11-17
function realpath(resolvedPath: string): string {
try {
return fs.realpathSync(resolvedPath);
} catch {
return resolvedPath;
}
}The result of realpath() is returned as the resolved id from resolveId (compareTransitiveExternalResolutions returns importerRealpath at line 43, which flows out of the handler at lines 147/152). Everywhere else in this codebase, fs.realpathSync / fs.realpath.native output is wrapped in toSlash(...) before it becomes a module id, precisely because on Windows it returns backslash separators and the repo enforces canonical forward-slash ids (see plugins/og-asset-ownership.ts:6-9, plugins/import-meta-url.ts:304, plugins/extensionless-dynamic-import.ts:656, index.ts:1460). Returning a backslash id here would break the forward-slash id invariant that route logic, startsWith/split("/"), and generated import specifiers rely on.
Additionally, the codebase canonicalizes ids with fs.realpathSync.native (see utils/commonjs-loader.ts:25, server/prod-server.ts:118) because Rolldown keys canonical module ids off realpathSync.native. This plugin uses the non-native fs.realpathSync. On case-insensitive or otherwise-normalizing filesystems the non-native variant can yield a different string than what Rolldown will canonicalize to, which risks a duplicate-module id for the very copy we are trying to bundle distinctly.
Suggested fix: import toSlash from pathslash and use fs.realpathSync.native, e.g. return toSlash(fs.realpathSync.native(resolvedPath)) (with the same toSlash(path.resolve(...)) fallback pattern used elsewhere). The equality comparison at line 43 stays correct either way since both sides go through the same helper, but the returned id must be canonical.
The same applies to the CJS fallback: resolveTransitiveExternal (line 57) returns createRequire(importer).resolve(request), whose value is a native-separator path fed into the same compareTransitiveExternalResolutions → returned id.
2. No coverage / handling for the ESM-vs-CJS resolution difference (low, parity gap)
Next.js baseResolveCheck bundles when either the resolved path differs or the module format differs: if (baseRes !== res || isEsm !== baseIsEsm). This PR only compares paths, not the resolved format. If the importer and root resolve to the same file but Vite would pick different export conditions (import vs require) for that request, Next.js bundles and vinext would leave it external. This is likely a rare edge and may be acceptable, but it is a deliberate divergence from the referenced baseResolveCheck; worth a code comment noting the intentional gap so it is not mistaken for full parity.
3. Test only exercises POSIX symlink layout (low)
tests/externals-transitive.test.ts builds the fixture with node:path (correct per AGENTS.md) and relies on junction/symlinks. It runs on every platform, but the assertion path (returned realpath id) would exercise the Windows backslash behavior from finding #1. If finding #1 is fixed, this test would validate the conversion on a Windows runner; if not, it may surface the id-invariant issue there. Consider confirming behavior on Windows CI or gating/annotating accordingly.
Nits
transitive-externals.ts:141-156: the handler returns an IIFE-wrapped async closure. Returning anasync handler(or making the whole handler async) reads more directly; minor.- The comment at line 74 says it "Mirrors Next.js's
baseResolveCheck" — given finding #2, tightening this to "the path-equality portion of" would set accurate expectations.
What looks good
- Correct handling of the "root can't resolve" case (returns importer realpath → bundle), matching Next.js's
baseRes = nullbranch. - Proper gating: filters by the bare-specifier regex, checks
externalPackages.hasbefore any resolution, and skips\0/query/non-absolute importers — keeps the common request path cheap per the perf guidance. - Environment scoping via
applyToEnvironment(non-client only) and per-environment resolver caching throughcreateIdResolver. - The
nitroTraceDepsFromServerExternals→resolvedServerExternalPackagesrename is consistent across all use sites.
|
Review posted to PR #2887. Summary of findings: Review SummaryThe PR's approach is sound — it correctly mirrors Next.js Medium — Low — ESM/CJS format parity gap: Next.js Low — test only exercises POSIX symlink layout and depends on the returned realpath id; worth confirming Windows behavior (tied to the medium finding). Plus nits on the IIFE-wrapped async handler and the "mirrors baseResolveCheck" comment accuracy. Gating, environment scoping, resolver caching, the root-unresolvable branch, and the variable rename all look correct. |
|
/bigbonk review for issues |
Performance benchmarksCompared 0 improved · 0 regressed · 6 within ±1.5%
View detailed results and traces 🟢 improvement · 🔴 regression · ⚫ change below 1.5% · paired base/head |
Re-review: transitive external version preservationThe three prior findings from the last
The core approach still looks correct (resolve-from-importer vs resolve-from-root, bundle on divergence), and the E2E test passes locally. One new finding. 1. Plugin runs in dev — should be gated to
|
|
Review posted to PR #2887. SummaryThe three findings from the previous
I found one new medium issue: Plugin runs in dev; should be gated to Plus a low follow-on note that the test only covers the production path (which is correct), so dev behavior is untested either way. Everything else — canonicalization, cheap gating, resolver caching, the null-root branch, and the |
|
/bigbonk review for issues |
There was a problem hiding this comment.
Re-review: transitive external version preservation (b3a51d6)
The medium finding from the previous /bigbonk pass — the plugin running in dev — is resolved. Commit b3a51d6 adds apply: "build" to createTransitiveExternalsPlugin (transitive-externals.ts:101), so the enforce: "pre" resolveId hook no longer fires during vite dev. This is consistent with the adjacent createServerExternalsManifestPlugin() (server-externals-manifest.ts:83), and correct: dev/preview serve native resolution or pre-built output, so there is no output relocation to guard against there.
I re-verified the full change and the earlier findings; everything checks out. tests/externals-transitive.test.ts passes locally (2.4s), and vp check is clean on both changed source files.
Verified correct
apply: "build"+configResolvedordering —rootandresolvedServerExternalPackagesare both populated in the main plugin'sconfighook (index.ts:1976,index.ts:2588), which runs before this plugin'sconfigResolvedsnapshots them. No stale/undefined read.- Externalization override — Vite's SSR externalization matches on the bare specifier before resolution; returning an absolute resolved id from
enforce: "pre"demotes it out of the external check and into the bundle. The test empirically confirms depB's nestedshared-version@4.17.21and nestedpg@9.0.0are preserved distinctly from the root copies. - Default externals covered — the test exercises both a user-listed external (
shared-version) and a default external (pg), so the merged default+explicit set is proven. - Canonicalization —
realpath()returnstoSlash(fs.realpathSync.native(...))with atoSlash(...)fallback; ids are canonical forward-slash and match Rolldown's native canonicalization (prior finding, still good). - Cheap gating — bare-specifier regex
filter,externalPackages.hasshort-circuit, and\0/query/non-absolute importer skips all run before any resolver call. - Resolver split — per-environment
createIdResolvercaching and the import-vs-require selection match the(environment, id, importer)ResolveIdFncontract;this.environmentis captured at handler-call time inside the returned Promise. - Rename —
nitroTraceDepsFromServerExternals→resolvedServerExternalPackagesis complete with no dangling references.
Low (optional, unchanged from prior passes)
- ESM/CJS format divergence is documented (
transitive-externals.ts:82-85) as an intentional gap vs Next.js'sisEsm !== baseIsEsmcomparison — fine as-is. - Dev is now correctly untested because it is gated off; no dev test needed given
apply: "build".
No blocking issues. LGTM.
|
Review posted to PR #2887. SummaryThe PR is in good shape. The medium finding from the previous I re-verified the whole change:
No blocking issues found. The two low-severity items (ESM/CJS format divergence, dev being untested) are both already correctly documented or made moot by the |
|
/bigbonk review for issues |
|
@james-elicx Bonk workflow failed. Check the logs for details. View workflow run · To retry, trigger Bonk again. |
|
/bigbonk review for issues |
|
UnknownError: ProviderInitError |
|
/bigbonk review for issues |
There was a problem hiding this comment.
Review: transitive external version preservation (d5dfeb9)
I re-reviewed the full change independently. The approach faithfully mirrors the path-equality portion of Next.js's baseResolveCheck — resolve from the importer, resolve from the project root, and bundle the importer's nested copy when they diverge so output relocation cannot collapse two distinct package versions to the root copy. Verified locally:
tests/externals-transitive.test.tspasses (2.4s).vp checkis clean ontransitive-externals.tsand the test.
Prior findings — all resolved
- Canonicalization —
realpath()returnstoSlash(fs.realpathSync.native(...))with atoSlash(...)fallback (transitive-externals.ts:12-18). Ids are canonical forward-slash and match Rolldown's native canonicalization. Both sides of the equality at line 44 go through the same helper, so the comparison is symmetric while the returned id is canonical. - ESM/CJS format divergence — documented as an intentional gap (
transitive-externals.ts:82-86): Vite selects an import- or require-configured resolver up front rather than comparingisEsm !== baseIsEsm. Fine as-is. apply: "build"gating —transitive-externals.ts:102gates theenforce: "pre"resolveIdhook to builds, matching the siblingcreateServerExternalsManifestPlugin(). Correct: dev/preview use native resolution or pre-built output, so there is no output relocation to guard against.
Independently verified correct
configResolvedordering is safe —root(index.ts:1976) andresolvedServerExternalPackages(index.ts:2588) are both populated in the main plugin'sconfighook, which runs before this plugin'sconfigResolvedsnapshots them via thegetRoot/getExternalPackagesclosures. No stale/undefined read; the handler also guards every resolver var.- Externalization override — Vite's SSR externalization matches the bare specifier before resolution; returning an absolute resolved id from an
enforce: "pre"hook demotes the request out of the external check and into the bundle. The test empirically proves depB's nestedshared-version@4.17.21andpg@9.0.0stay distinct from the root3.10.1/8.0.0. - Default + explicit externals both covered — the test exercises a user-listed external (
shared-version) and a default external (pg), so the merged default+explicit set frommergeServerExternalPackagesis proven. - Cheap gating — the
BARE_PACKAGE_SPECIFIER_REfilter(whose[^?#]*subpath excludes queried sources), theexternalPackages.hasshort-circuit, and the\0/query/non-absolute-importer skips all run before any resolver call, keeping the hot path light per the perf guidance. - Resolver split — per-environment
createIdResolverimport/require caching, themoduleMode(resolveOptions.kind)selection, and therequire-only NodecreateRequirefallback (never used for ESM, avoiding require-only export selection) match theResolveIdFncontract.this.environmentis read at handler-call time inside the returned Promise. nullroot-resolution branch — returning the importer realpath when the root cannot resolve (compareTransitiveExternalResolutionsline 43; CJS fallback lines 63-71) mirrors Next.js'sbaseRes = nullcase: an importer-only package must be bundled or it will not exist beside a relocated server bundle.- No plugin conflict —
createServerExternalsManifestPlugin()is awriteBundle/enforce: "post"collector, not a resolver, so it does not race this hook. - Rename —
nitroTraceDepsFromServerExternals→resolvedServerExternalPackagesis complete across all use sites (index.tsdeclaration,configassignment, NitrotraceDepsmerge) with no dangling references.
Nits (non-blocking)
transitive-externals.ts:149returns an IIFE-wrapped async closure. Anasync handlerreads more directly, but the IIFE preserves the cheap synchronous guard/filter path before any await, which is a reasonable tradeoff — leave as-is.
No blocking issues. LGTM.
|
Review posted to PR #2887. SummaryI independently re-reviewed the whole change ( All three prior
Independently verified correct:
Only a non-blocking nit about the IIFE-wrapped async handler, which is actually a reasonable tradeoff since it preserves the cheap synchronous guard path before any |
|
NO FINDINGS — full independent closure review of exact head I reviewed the complete diff and the queried-importer follow-up. The build-only, non-client resolver strips Vite query/hash suffixes from absolute importer ids, compares canonical importer-relative and project-root package instances, and returns an absolute id only when the nested copy differs so Rolldown bundles that copy instead of collapsing it to the root external. Same-instance packages remain external. The package filter, import/require resolver split, symlink realpath handling, missing-root fallback, Nitro trace-dependency rename, and dev/preview/build boundaries do not expose a correctness or regression gap. Validation on this exact SHA:
|
1 participant
Summary
Failure mapping
Fixes the non-cache externals-transitive failure from Actions run 31439707085, job 93624401572:
Validation
Refs #1503.