fix: merge top-level optimizeDeps with per-environment config

[Jbithell]

Summary

Fixes #538

vinext's config() hook was creating per-environment optimizeDeps objects from scratch, discarding any exclude or include entries that other Vite plugins (e.g. @lingui/vite-plugin) had added to the top-level config.optimizeDeps during their own config hooks.

This PR captures the incoming config.optimizeDeps.exclude and config.optimizeDeps.include arrays before building the per-environment configs, then merges them (with deduplication via Set) into each environment:

• rsc: exclude now includes incoming excludes + ["vinext", "@vercel/og"]
• ssr: exclude now includes incoming excludes + ["vinext", "@vercel/og"]
• client: exclude now includes incoming excludes + ["vinext", "@vercel/og", ...serverExternalPackages]; include now includes incoming includes + React packages

Changes

• packages/vinext/src/index.ts — Read config.optimizeDeps?.exclude and config.optimizeDeps?.include at the top of the environments block, then spread them into each environment's optimizeDeps using [...new Set([...incoming, ...vinextOwn])]
• tests/build-optimization.test.ts — New test that passes optimizeDeps.exclude and optimizeDeps.include via the mock config (simulating an earlier plugin like @lingui/vite-plugin) and verifies those entries appear in all three environments alongside vinext's own entries

Test plan

• New unit test: passes top-level optimizeDeps.exclude (@lingui/macro, @lingui/core/macro) and optimizeDeps.include (some-lib) via mock config, verifies they appear in rsc/ssr/client environments alongside vinext's own entries
• Existing optimizeDeps.exclude tests still pass (66/66 in build-optimization.test.ts)
• pnpm run fmt:check — passes
• pnpm run lint — 0 warnings, 0 errors
• pnpm run typecheck — passes

[james-elicx]

/bigbonk review

[pkg-pr-new]

Open in StackBlitz

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

commit: b37f89d

[ask-bonk]

Good fix for a real bug. The approach of capturing incoming optimizeDeps and merging with Set is clean and correct. A couple of observations below — one is a gap that affects Pages Router users with the same setup, the other is a minor test coverage note.

[ask-bonk]

This correctly handles the App Router environments, but the same class of bug exists at the top-level optimizeDeps (line ~1294) used for Pages Router builds:

optimizeDeps: {
  exclude: ["vinext", "@vercel/og"],
},

Vite's config() hook merging replaces arrays, so if @lingui/vite-plugin added entries to config.optimizeDeps.exclude, they get overwritten by this return value for Pages Router builds too.

This is pre-existing (not introduced by this PR), but since you're fixing this class of issue, it'd be worth applying the same merge pattern there as well. Something like:

optimizeDeps: {
  exclude: [...new Set([...incomingExclude, "vinext", "@vercel/og"])],
},

You'd need to move the incomingExclude/incomingInclude capture above the if (hasAppDir) block so it's available in both code paths. Up to you whether to include it in this PR or a follow-up — the current fix is correct for the App Router case.

[ask-bonk]

Nice test coverage. One minor addition to consider: asserting that @vercel/og is still present in the excludes would confirm the merge didn't accidentally drop vinext's own entries beyond just "vinext". Currently only "vinext" is checked as the "vinext's own excludes" representative.

Also, it might be worth adding a negative assertion that there are no duplicates in the array, to validate the Set dedup is working:

Suggested change

	for (const envName of ["rsc", "ssr", "client"]) {
	for (const envName of ["rsc", "ssr", "client"]) {
	const envExclude = result.environments[envName].optimizeDeps?.exclude;
	expect(envExclude, `${envName} should contain @lingui/macro`).toContain("@lingui/macro");
	expect(envExclude, `${envName} should contain @lingui/core/macro`).toContain(
	"@lingui/core/macro",
	);
	// vinext's own excludes should still be present
	expect(envExclude, `${envName} should contain vinext`).toContain("vinext");
	expect(envExclude, `${envName} should contain @vercel/og`).toContain("@vercel/og");
	// No duplicates (Set dedup works)
	expect(new Set(envExclude).size, `${envName} should have no duplicate excludes`).toBe(envExclude.length);

[ask-bonk]

Good idea to simulate entries that a real plugin like @lingui/vite-plugin would add. Consider also testing the dedup path: what happens when the incoming config contains an entry that vinext also adds (e.g. exclude: ["vinext", "@lingui/macro"])? The Set should dedup it, but an explicit test case would document that behavior.

[ask-bonk]

Review posted successfully. Here's a summary of the feedback I left on PR #539:

Overall: Good fix for a real bug. The approach of capturing incoming optimizeDeps and merging via Set is clean and correct for the App Router environments.

Three comments posted:

1. Pages Router gap (index.ts:1348) — The same overwrite bug exists at the top-level optimizeDeps (~line 1294) used for Pages Router builds. Vite's config() hook array merging replaces rather than concatenates, so @lingui/vite-plugin entries would be dropped there too. Suggested moving the incomingExclude/incomingInclude capture above the if (hasAppDir) block and applying the same merge pattern to the Pages Router path (either in this PR or a follow-up).

2. Test coverage suggestion (build-optimization.test.ts:169) — Suggested also asserting @vercel/og is present (not just "vinext") and adding a no-duplicates assertion to validate the Set dedup is working.

3. Dedup test case (build-optimization.test.ts:161) — Suggested testing the dedup path explicitly by including an entry like "vinext" in the incoming excludes to document the behavior when incoming config overlaps with vinext's own entries.

github run

[james-elicx]

/bigbonk re-review

[ask-bonk]

Good fix — the core approach is correct and addresses a real issue. The previous review's suggestions (dedup testing, @vercel/og assertions, Pages Router merge) have all been incorporated. Two remaining items: a CI failure and a minor gap in the top-level merge.

CI: Format check is failing on tests/build-optimization.test.ts. Run pnpm run fmt and push the fix.

[ask-bonk]

This replaces the entire optimizeDeps object with only exclude, which drops incomingInclude (and any other properties like entries or esbuildOptions) that earlier plugins may have set on config.optimizeDeps for Pages Router builds.

The App Router path correctly merges both incomingExclude and incomingInclude (the latter into the client environment). For consistency, the top-level config should preserve incoming include too:

Suggested change

	exclude: [...new Set([...incomingExclude, "vinext", "@vercel/og"])],
	viteConfig.optimizeDeps = {
	exclude: [...new Set([...incomingExclude, "vinext", "@vercel/og"])],
	...(incomingInclude.length > 0 ? { include: incomingInclude } : {}),
	};

This way if a plugin like @lingui/vite-plugin sets both exclude and include at the top level, Pages Router builds preserve both.

[ask-bonk]

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

Overall: The fix is correct and well-tested. Previous review feedback has been fully incorporated. Two items remain:

1. CI Format failure — tests/build-optimization.test.ts has formatting issues. The author needs to run pnpm run fmt and push.

2. incomingInclude dropped for Pages Router (index.ts:1339) — The top-level viteConfig.optimizeDeps assignment only merges exclude, replacing the entire object and silently dropping any include entries from earlier plugins. This only affects Pages Router builds (App Router correctly merges incomingInclude into the client environment). Posted a suggestion to conditionally spread incomingInclude into the top-level config.

github run

[james-elicx]

/bigbonk re-review

[ask-bonk]

Good fix. The approach of capturing incoming optimizeDeps and merging via Set is correct and handles the core issue well. The previous review feedback (Pages Router merge, dedup testing, @vercel/og assertions, incomingInclude for Pages Router) has all been addressed.

CI is mostly green (Format, Lint, Typecheck all pass; remaining checks are still pending but the earlier failures have been resolved). Two minor observations below, neither blocking.

[ask-bonk]

Nit: this only preserves exclude and include from incoming config.optimizeDeps, silently dropping any other properties that earlier plugins might have set (e.g. entries, esbuildOptions, needsInterop). A more defensive approach would spread the entire incoming object:

Suggested change

	};
	viteConfig.optimizeDeps = {
	...config.optimizeDeps,
	exclude: [...new Set([...incomingExclude, "vinext", "@vercel/og"])],
	...(incomingInclude.length > 0 ? { include: [...new Set([...incomingInclude])] } : {}),
	};

This is pre-existing (the original code also only set exclude), so not blocking — just something to consider for robustness.

[ask-bonk]

Minor: the existing Pages Router test now passes optimizeDeps.exclude in the mock config but doesn't test include preservation for Pages Router. The implementation does conditionally spread incomingInclude for Pages Router (line 1340 of index.ts), but there's no test asserting it. Consider adding include: ["some-pages-lib"] to this mock config and verifying result.optimizeDeps?.include contains it — would give full coverage of the Pages Router merge path added at line 1338-1341.

[ask-bonk]

Review posted. Here's a summary:

Approved with two non-blocking observations:

1. index.ts:1341 — Incoming optimizeDeps properties beyond exclude/include are dropped. The new top-level assignment only preserves exclude and include, silently discarding any other properties earlier plugins may have set (e.g. entries, esbuildOptions). Suggested spreading the full incoming config.optimizeDeps object as a base. This is pre-existing behavior, so not blocking.

2. build-optimization.test.ts:120 — No test coverage for Pages Router include merge. The implementation conditionally spreads incomingInclude into Pages Router's top-level config, but the existing Pages Router test only passes exclude in the mock config. Adding an include entry would fully cover that code path.

The core fix is correct — it captures incoming optimizeDeps.exclude/include before building per-environment configs and merges them with deduplication via Set. All previous review feedback has been incorporated. CI checks that have completed so far (Format, Lint, Typecheck, several E2E projects) are passing.

github run