fix(build): externalize native ESM server packages#1993
Conversation
commit: |
NathanDrake2406
force-pushed
the
nathan/esm-externals-parity
branch
3 times, most recently
from
a828411 to
fe42289
Compare
NathanDrake2406
force-pushed
the
nathan/esm-externals-parity
branch
2 times, most recently
from
117b914 to
2e5eeeb
Compare
|
/bigbonk review for correctness and parity issues |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
Reviewed for correctness and Next.js parity. The core approach is sound and the fixture is a faithful port of the upstream esm-externals test. The Pages-Router native-ESM detection and the process.browser define split both look correct. A few parity gaps and a redundancy worth discussing:
1. serverExternalPackages vs transpilePackages precedence diverges from Next.js (parity). Next.js (webpack-config.ts) does two things this PR doesn't: (a) it throws if a package appears in both lists, and (b) when computing optOutBundlingPackages it filters out anything in transpilePackages — so transpile always wins. This PR checks serverExternalPackages first and returns external: true before the transpilePackages check, so if a package is in both, vinext externalizes it (the opposite of Next.js). The fixture keeps the two sets disjoint so this isn't exercised by tests. Consider checking transpilePackages first, and/or adding the conflict validation in next-config.ts.
2. serverExternalPackages externalization appears redundant with existing ssr.external wiring. index.ts already merges serverExternalPackages into userSsrExternal and propagates it to both the RSC (~L2195) and SSR (~L2231) resolve.external configs. The new plugin's explicit branch (server-node-externals.ts L275-278) seems to duplicate that. If the existing path is insufficient (e.g. timing/enforce ordering), a comment explaining why both are needed would help; otherwise this branch may be removable, leaving the genuinely new Pages-Router native-ESM detection as the plugin's purpose.
3. Native-ESM detection is heuristic and entry-only. hasNodeUnsupportedRelativeImport text-scans only the resolved entry file's direct relative imports. A native-ESM entry that re-exports through a .js chain importing CSS/assets transitively would still be externalized and could fail at runtime under Node. This matches the PR's documented "conservative" intent, but the regex also scans strings/comments and could false-positive (forcing bundling) on import-like text inside template literals. Acceptable as a safety bias, just noting the limitation.
No blocking issues; the divergences in (1) are the most important for parity.
| if (explicitServerExternal) { | ||
| return { id, external: true }; | ||
| } | ||
| if (options.getTranspilePackages().includes(packageName)) return null; |
There was a problem hiding this comment.
Parity: Next.js gives transpilePackages precedence over serverExternalPackages (and actually throws if a package is in both). Here serverExternalPackages is checked first and short-circuits with external: true, so a package present in both lists gets externalized — the opposite of Next.js. Consider checking transpile first:
| if (options.getTranspilePackages().includes(packageName)) return null; | |
| if (options.getTranspilePackages().includes(packageName)) return null; | |
| const explicitServerExternal = options.getServerExternalPackages().includes(packageName); | |
| if (explicitServerExternal) { | |
| return { id, external: true }; | |
| } |
The fixture keeps the two sets disjoint, so the current ordering isn't caught by tests. See webpack-config.ts: optOutBundlingPackages = EXTERNAL_PACKAGES.concat(serverExternalPackages).filter((pkg) => !finalTranspilePackages?.includes(pkg)).
|
|
||
| const explicitServerExternal = options.getServerExternalPackages().includes(packageName); | ||
| if (explicitServerExternal) { | ||
| return { id, external: true }; |
There was a problem hiding this comment.
serverExternalPackages is already merged into userSsrExternal in index.ts and propagated to both the RSC and SSR resolve.external configs (around L2111-2116 / L2195 / L2231). This explicit branch looks like it duplicates that existing externalization. If the existing wiring is insufficient (ordering / enforce: "pre" reasons), a short comment here would clarify why both paths are needed; otherwise this could be dropped, leaving the Pages-Router native-ESM detection below as the plugin's distinct value.
| return ext === ".mjs" || (ext === ".js" && metadata?.type === "module"); | ||
| } | ||
|
|
||
| function hasNodeUnsupportedRelativeImport(source: string): boolean { |
There was a problem hiding this comment.
This text scan only inspects the resolved entry file's direct relative imports — a native-ESM entry that re-exports through a .js file which transitively imports CSS/assets would still be externalized and could throw under Node at runtime. Also, the regex matches import/require-like text anywhere in the source (including strings/comments), which can false-positive and force bundling. Both are acceptable conservative biases given the PR's stated goal, but worth a comment noting the entry-only scope so future readers don't assume it's a full transitive check.
| ); | ||
| const serverExternalPackages = topLevelServerExternalPackages ?? legacyServerComponentsExternal; | ||
| const transpilePackages = Array.isArray(config.transpilePackages) | ||
| ? readStringArray(config.transpilePackages) |
There was a problem hiding this comment.
Optional parity nicety: Next.js throws when a package appears in both transpilePackages and serverExternalPackages ("The packages specified in the 'transpilePackages' conflict with the 'serverExternalPackages'"). Adding the same validation here would surface misconfiguration early and make the precedence question moot.
|
Review posted successfully to PR #1993. Here's a summary of what I found: Review SummaryThe PR's core approach is sound — the resolver correctly distinguishes native ESM externals from invalid ESM condition targets, the test fixture is a faithful port of upstream Next.js's I posted one COMMENT-level review with 4 inline comments. Key findings: 1. Parity divergence — 2. Redundancy. 3. Heuristic limitation. No blocking issues were found — the parity divergence in (1) is the most significant. |
Vinext bundled almost every server node_modules dependency during production builds. That diverged from Next.js Pages Router externalization and made valid ESM externals resolve sentinel dynamic imports during build. The resolver treated app serverExternalPackages and valid Pages ESM packages the same as ordinary bundled dependencies. This broke the Next.js esm-externals deploy fixture before runtime. Add a server build resolver that honors serverExternalPackages and externalizes only Pages-side native ESM packages that Node can import directly, while leaving invalid ESM condition targets bundled. Mirror Next's process.browser define split for client and server bundles. Port the upstream fixture coverage into Vitest and Playwright, and verify the upstream deploy test passes.
NathanDrake2406
force-pushed
the
nathan/esm-externals-parity
branch
from
2e5eeeb to
b332e28
Compare
2 participants
Overview
esm-externalsdeploy contract for production builds.serverExternalPackagesand Pages-side native ESM packages that Node can import directly.packages/vinext/src/plugins/server-node-externals.ts,packages/vinext/src/index.ts,tests/esm-externals.test.ts,tests/e2e/app-router/nextjs-compat/esm-externals.browser.spec.tsimport("fail")entries.Why
Next.js server builds do not treat every server
node_modulesimport as something to bundle. The resolver distinguishes native ESM externals from invalid ESM condition targets, andserverExternalPackagesexplicitly opts app packages out of bundling. Vinext was force-bundling these package entries, so valid native ESM packages were analyzed by Rolldown and tripped the upstream fixture's sentinelimport("fail")code..mjsor.jsundertype: "module".importcondition points at non-ESM-flagged.jsshould remain bundled in the Turbopack branch.Worldexpectation forinvalid-esm-package.serverExternalPackagesis an explicit opt-out from server bundling.process.browserdifferently in client and server bundles.process.browser: truefor client and overlaysfalsefor non-client environments.What changed
/static,/ssr,/ssgwith valid ESM packagesimport("fail")..jstarget remains bundled and rendersWorld, matching the Turbopack branch./server,/clientpackages inserverExternalPackagesprocess.browserReferenceError: process is not defined.process.browserastrue, matching Next's define environment.Maintainer review path
packages/vinext/src/plugins/server-node-externals.tsReview the resolver gates: build-only, server-only, explicit package externalization, Pages-side native ESM check, and conservative non-JS import guard.
packages/vinext/src/index.tsReview plugin registration placement and the
process.browserdefine split.tests/helpers/esm-externals-fixture.tsReview the ported fixture package shapes against upstream package exports and route structure.
tests/esm-externals.test.tsandtests/e2e/app-router/nextjs-compat/esm-externals.browser.spec.tsReview the SSR and browser assertions against the upstream Turbopack expected strings.
Validation
Added coverage:
tests/esm-externals.test.tscovers production build plus SSR output for the ported fixture.tests/e2e/app-router/nextjs-compat/esm-externals.browser.spec.tscovers hydrated browser text for the same routes.Commands run:
The upstream deploy run passed all 10 assertions for
test/e2e/esm-externals/esm-externals.test.ts.Risk / compatibility
serverExternalPackagesnow affects the build resolver for Node production builds in the tested server environments.Non-goals
esm-externalsparity contract.References
esm-externalstestresolveExternalserverExternalPackagesmerged into the bundling opt-out set.process.browserserverExternalPackagesdocs