fix(pages): match ESM externals behavior#2237
Conversation
|
/bigbonk review for issues |
commit: |
|
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: fix(pages): match ESM externals behavior
Reviewed the new pages-node-externals plugin against the Next.js reference (build/handle-externals.ts, build/webpack-config.ts). The approach faithfully ports Next.js's resolveExternal semantics (import/require condition selection, base-resolve check for nested versions, strict-require throw, transpile/bundle opt-outs). Tests pass locally (tests/esm-externals.test.ts + tests/next-config.test.ts, 214 passed) and vp check is clean on the changed files. The transpilePackages ↔ serverExternalPackages conflict error string matches Next.js verbatim. Nice work.
A few issues worth considering before merge — none are blockers, but the first is a real parity gap.
1. Strict-require throw is bypassed by bundlePagesRouterDependencies / transpilePackages (parity gap)
In Next.js, the "ESM packages need to be imported" error is thrown before the bundling decision, so require() of an ESM-only package throws even when bundlePagesRouterDependencies: true or the package is in transpilePackages:
// handle-externals.ts
// ESM externals can only be imported (and not required).
if (!isEsmRequested && isEsm && !looseEsmExternals && !isLocal) {
throw new Error(`ESM packages (${request}) need to be imported. ...`)
}
// ... bundlePagesRouterDependencies / transpilePackages handling happens AFTER thisIn pages-node-externals.ts, the early return at line 167 short-circuits the require-request handling (lines 187–199), so vinext silently bundles instead of throwing:
if (options.getEsmExternals() === false || options.getBundlePagesRouterDependencies()) {
return null; // <-- returns before the strict-require throw
}
// ... transpilePackages also returns null below, before the throwSo require("esm-only-pkg") throws in vinext only when none of esmExternals:false / bundlePagesRouterDependencies:true / transpilePackages apply, whereas Next.js throws regardless (except in loose mode / for local requests). Worth either matching Next.js (evaluate the strict-require throw first) or documenting this as a deliberate divergence per AGENTS.md.
2. transform hook has no native filter (perf)
AGENTS.md (Performance Best Practices) calls out filtering Vite hooks before JS runs. The transform handler runs for every non-client module during build and does a code.includes("require") scan on each:
transform: {
order: "pre",
handler(code, id) {
if (!isBuild || !options.isEnabled() || this.environment?.name === "client") return null;
if (!code.includes("require")) return null;
...Consider a native filter (e.g. on code: { include: /\brequire\s*\(/ } and/or excluding node_modules that can't contain bare require to packages) so the handler doesn't run on every module. Minor, but it's exactly the pattern the repo guidelines flag.
3. hasNodeUnsupportedRelativeImport only scans the entry file (heuristic limitation)
canNodeImport(entry) && !hasNodeUnsupportedRelativeImport(entry) only inspects the resolved entry module, not its transitive relative imports. A native-ESM package whose entry imports ./foo.js (with extension) but where ./foo.js imports ./bar (no extension) would be externalized and then fail at Node runtime. This matches the practical risk surface for most packages, but it's a shallow check — fine to leave, worth a comment noting the limitation.
4. MODULE_SPECIFIER_RE misses minified export{...}from"..." (edge)
The regex requires whitespace before from, so export{a}from"./rel" (no space, as emitted by some minifiers) isn't matched:
"export {a}from\"./nospace\"" => [] // not detected
Consequence: a minified native-ESM package using extensionless re-exports could be falsely externalized. Very narrow, but the \s+...from\s* requirement could be relaxed to \s*from\s* after a }/identifier.
Minor
resolveExternal's base-resolve check compares bothbaseRes !== resandisEsm !== baseIsEsm; the root-resolution check here (lines 218–228) only compares the resolved path, not ESM-ness. Unlikely to matter in practice.getTranspilePackages().includes(packageName)matches only by package name, whereas Next.js'sisResourceInPackagesresolves package dirs (catches transitively-located resources). Acceptable simplification.
Overall the change is well-tested and closely tracks Next.js. Item 1 is the one I'd most want addressed (or explicitly documented) before merge.
|
Review posted on PR #2237. SummaryI reviewed PR #2237 ( The implementation is solid and closely ports Next.js's
Plus two minor notes (ESM-ness not compared in the root-resolution check; |
Performance benchmarksCompared 1 improved · 1 regressed · 4 within ±1.5%
View detailed results and traces 🟢 improvement · 🔴 regression · ⚫ change below 1.5% · paired base/head |
1 participant
Summary
Validation
vp test run tests/esm-externals.test.ts tests/next-config.test.ts— 214 passedvp checkandgit diff --checkNext.js parity
Targets the failures in
test/e2e/esm-externals/esm-externals.test.tsfrom deploy-suite run 27858251903. Investigation and reproduction used vinext-owned fixtures; no raw Next.js E2E was run locally.