fix: prevent React externalization leak in App Router SSR

[StringerBell69]

Resolves #848

The Bug
When both top-level ssr.external and per-environment environments.ssr are set, Vite merges the top-level config into the environment. This caused React packages from ssr.external to leak into the App Router SSR environment's external list, making Node.js resolve React from vinext's package scope instead of the project root.

This bypassed resolve.dedupe and produced dual React instances in split-install topologies (npm link / bun link), resulting in 'Invalid hook call' errors during SSR.

The Fix
Only include React in top-level ssr.external when App Router is NOT active. When App Router is active, per-environment configs already define the correct external lists without React.

Testing
Adds a regression test verifying:

• App Router: React not in top-level ssr.external
• Pages Router: React still externalized (CJS compat)
• resolve.dedupe always includes React packages

[pkg-pr-new]

Open in StackBlitz

npm i https://pkg.pr.new/vinext@1066

commit: b788844

[james-elicx]

/bigbonk review

[ask-bonk]

Review: fix React externalization leak in App Router SSR

The problem analysis is correct — Vite merges top-level ssr.* config into environments.ssr, so having React in ssr.external causes it to leak into the App Router SSR environment. The fix to conditionally exclude React from top-level ssr.external when App Router is active is the right approach.

However, the pages_ssr environment addition has several issues that need to be addressed before merging.

Summary of concerns

1. pages_ssr environment is missing optimizeDeps, build, and consumer config — it's a bare shell with only resolve.external. The ssr environment has optimizeDeps (with entries, exclude) and build (with outDir, input). The pages_ssr environment has none of this, which means Vite will use defaults and it may not behave correctly.

2. pages_ssr is only created in dev (isDev) but getPagesRunner falls back to ssr — this means in dev with a hybrid app+pages project, Pages Router requests will use pages_ssr (with React externalized), but the ssr environment (which the App Router SSR entry uses) won't have React externalized. That part is correct. But in production builds, there is no pages_ssr and the Pages Router production server (server/prod-server.ts) uses a different code path entirely. The dev-only scoping is fine logically but should be documented.

3. Test coverage gap — the tests only check config.ssr.external (the top-level resolved config), not the per-environment resolved config. The actual bug is about what ends up in environments.ssr.resolve.external after Vite's merge. Testing the resolved environment config would be a stronger regression test.

4. Pages Router test is weak — wrapping the assertion in if (Array.isArray(ssrExternal)) means the test silently passes if ssrExternal is undefined or some other non-array type. This should be an unconditional assertion.

[ask-bonk]

The pages_ssr environment is missing optimizeDeps and build config that the sibling ssr environment receives. Without optimizeDeps.entries and optimizeDeps.exclude, Vite won't discover Pages Router dependencies at startup in this environment, which could cause re-optimization cascades.

Also, there's no consumer set — is it inheriting the correct server consumer? And there's no build block — is that intentional since this is dev-only? If so, a comment explaining this would help.

Finally, the pages_ssr environment doesn't exist during production builds (guarded by isDev). The AGENTS.md file warns about dev/prod parity:

When fixing a bug in any of these files, check whether the same bug exists in the others. Do not leave known bugs as "follow-ups" — fix them in the same PR.

For production builds of hybrid apps (both app/ and pages/), the Pages Router production server handles its own SSR. Is the React externalization leak also a problem in production builds? If not, this should be documented in a comment explaining why dev-only is sufficient.

[ask-bonk]

This test checks config.ssr.external (the top-level merged config), but the actual bug is about what Vite merges into the per-environment config (environments.ssr). A stronger regression test would inspect the resolved environment config to verify React doesn't appear there.

For example, after the server is created you could check:

Suggested change

	for (const pkg of reactPackages) {
	if (Array.isArray(ssrExternal)) {
	expect(ssrExternal).not.toContain(pkg);
	}
	// If ssrExternal is true (externalize everything), that's also wrong for App Router
	expect(ssrExternal).not.toBe(true);
	}
	// The resolved per-environment SSR config should NOT contain React.
	// This is the actual config Vite uses after merging top-level ssr.*
	// into environments.ssr.
	const ssrEnvConfig = appServer.environments?.ssr;
	const resolvedExternal = ssrEnvConfig?.config?.resolve?.external;
	const reactPackages = ["react", "react-dom", "react-dom/server"];
	for (const pkg of reactPackages) {
	if (Array.isArray(resolvedExternal)) {
	expect(resolvedExternal).not.toContain(pkg);
	}
	}
	// Also verify top-level ssr.external doesn't contain React
	const ssrExternal = appServer.config.ssr.external;
	for (const pkg of reactPackages) {
	if (Array.isArray(ssrExternal)) {
	expect(ssrExternal).not.toContain(pkg);
	}
	expect(ssrExternal).not.toBe(true);
	}

(The exact API to read the resolved environment config may differ — check server.environments.ssr.config or similar.)

[ask-bonk]

This assertion is guarded by if (Array.isArray(ssrExternal)), which means the test silently passes if ssrExternal is undefined or any non-array value. The test should unconditionally assert the expected shape:

Suggested change

	if (Array.isArray(ssrExternal)) {
	expect(ssrExternal).toContain("react");
	expect(ssrExternal).toContain("react-dom");
	}
	expect(Array.isArray(ssrExternal)).toBe(true);
	expect(ssrExternal).toContain("react");
	expect(ssrExternal).toContain("react-dom");
	expect(ssrExternal).toContain("react-dom/server");

[ask-bonk]

Same issue as the Pages Router test — the if (Array.isArray(ssrExternal)) guard means this assertion is skipped if ssrExternal is not an array. Given this test's purpose is to catch a regression, it should fail loudly if the config shape is unexpected:

Suggested change

	for (const pkg of reactPackages) {
	if (Array.isArray(ssrExternal)) {
	for (const pkg of reactPackages) {
	expect(Array.isArray(ssrExternal)).toBe(true);
	expect(ssrExternal).not.toContain(pkg);

[ask-bonk]

Good addition — the pages_ssr fallback chain makes sense. Worth adding a brief inline comment explaining why pages_ssr is preferred over ssr here (because ssr in App Router mode doesn't externalize React, but Pages Router needs CJS React externalized for correct module resolution).

[ask-bonk]

Review submitted. Here's a summary of the feedback:

Requested changes on PR #1066:

1. Incomplete pages_ssr environment config — The new environment only has resolve.external but is missing optimizeDeps (entries, exclude) and build config that the sibling ssr environment receives. This could cause dep optimizer issues in dev.

2. Dev/prod parity needs documentation — pages_ssr is dev-only (isDev guard). This is likely correct since production builds use prod-server.ts, but it should be documented per the repo's guidelines about checking all server code paths.

3. Tests check wrong config level — The tests verify config.ssr.external (top-level), but the actual bug is about what Vite merges into the per-environment config. Testing the resolved environment config would catch the actual regression.

4. Weak assertions — Both the App Router and Pages Router tests wrap assertions in if (Array.isArray(ssrExternal)), meaning they silently pass if the config shape is unexpected. These should be unconditional assertions.

5. Missing comment on getPagesRunner fallback — The new pages_ssr priority in the fallback chain is correct but should explain why it's preferred over ssr (React CJS externalization).

github run