Review: vinext:transitive-externals plugin

Good work — this is a well-scoped, cleanly tested port of the Next.js baseResolveCheck logic. The core resolution function is correct, the test fixture layout mirrors Next.js's externals-transitive fixture, and the plugin guard chain is solid. A few things to address:

Issues

1. Missing environment guard — plugin runs unnecessarily in the client environment

resolve.external is only configured on rsc and ssr environments (and skipped entirely for Cloudflare/Nitro). The client environment never externalizes packages, so the plugin's resolveId hook does unnecessary work there. Other plugins in the codebase (e.g. optimize-imports.ts, async-hooks-stub.ts) already follow this pattern.

Add an early return at the top of resolveId:

// packages/vinext/src/plugins/transitive-externals.ts:122
resolveId(source, importer) {
  // Only relevant in server environments where resolve.external is active.
  if (this.environment?.name === "client") return null;
  // ...rest
}

Not a correctness bug (resolving to an absolute path in the client env would just bundle it normally), but it avoids per-import overhead in the client build.

2. configResolved is called on every runResolveId invocation in tests

In the test helper runResolveId (line 150-154), configResolved is called every time the helper is invoked, not once per plugin instance. This means a new rootResolver (via createRequire) is created on every call and the externalSet is rebuilt every time. This works because the plugin closure reassigns both variables, but it's wasteful and doesn't reflect how Vite actually calls the lifecycle. Consider extracting configResolved invocation to happen once after _createPluginForTest, and having runResolveId only call the resolveId hook.

3. Subpath import resolution (lodash/package.json, @scope/lib/utils)

The plugin correctly extracts pkgName from subpath imports (lines 131-136), but there's no test for subpath imports. Consider adding a test case for e.g. "lodash/package.json" or "lodash/cloneDeep" from dep-b to verify the extraction + resolution works end-to-end.

4. Cloudflare/Nitro targets — plugin is a no-op but still runs

On Cloudflare/Nitro targets, serverExternalPackages defaults to [] and no resolve.external is configured, so externalSet.size === 0 exits early at line 126. This is fine — the guard handles it. But worth adding a one-liner note in the plugin's JSDoc that the plugin is intentionally inert on bundled runtimes.

Observations (non-blocking)

Port accuracy vs Next.js baseResolveCheck: The Next.js version also checks ESM vs CJS resolution mode (isEsm !== baseIsEsm — see baseRes !== res || isEsm !== baseIsEsm). The vinext port only checks path identity, which is correct for Vite's module graph (Vite handles ESM/CJS resolution internally via its own resolver), but worth documenting the deliberate omission.

realpathSync fallback pattern: The triple try-catch in resolveTransitiveExternal (lines 46-50, 55-64) is defensive and correct. Symlinked node_modules (pnpm, Bun) will hit the realpathSync path, and the fallback ensures broken symlinks don't crash the build. No concern here.

createRequire(importer) per call: Each resolveId invocation creates a fresh createRequire(importer). This is unavoidable since the importer changes per call, and createRequire is lightweight. No concern.

Overall this is a clean, well-motivated change. The issues above are minor — the core logic is correct and well-tested.

Review: vinext:transitive-externals plugin (post-feedback)

All four items from the previous review have been addressed in 8784cb8:

1. Client environment guard — added at line 136 with clear comment. Matches the this.environment?.name === "client" pattern used by async-hooks-stub.ts, optimize-imports.ts, and others.

2. configResolved lifecycle — setupPlugin() now calls configResolved once and reuses the initialized instance across all resolveId calls. Matches Vite's actual lifecycle.

3. Subpath import coverage — Two new test cases (lodash/cloneDeep at line 246, @scope/lib/utils at line 278) exercise the pkgName extraction path for both unscoped and scoped subpath imports.

4. Cloudflare/Nitro JSDoc — Plugin-level JSDoc (lines 6-14) explicitly documents the no-op behavior on bundled runtimes.

One non-blocking observation

path.sep vs / in node_modules check (line 161) — The rest of the codebase consistently uses hardcoded /node_modules/ for path checks in Vite plugin hooks (index.ts:948, client-reference-dedup.ts:30, fonts.ts:1126, etc.). Vite normalizes module IDs to forward slashes on all platforms, so path.sep is unnecessary here and is an outlier. It works correctly (since path.sep is / on Linux/macOS, and Vite normalizes on Windows), but consider aligning with the codebase convention in a follow-up. The only place path.sep is used with node_modules is build/standalone.ts, where it operates on real OS filesystem paths from fs.cpSync, not Vite module IDs.

That said, the importer argument to resolveId is an absolute filesystem path that may not be Vite-normalized in all edge cases (e.g. when a non-Vite resolver provides the importer), so path.sep is arguably the more defensive choice here. Fine either way.

Verdict

Clean, well-motivated port of baseResolveCheck. Core resolution logic is correct, test coverage is thorough, all review feedback addressed. CI is green (Check, Vitest, E2E all passing). Ship it.