fix(app-router): use experimental React channel#2372
Conversation
|
/bigbonk review for issues |
commit: |
|
|
/bigbonk review for issues |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: fix(app-router): use experimental React channel
Solid, focused PR. I checked out the branch, ran the unit + production build/preview tests (tests/experimental-react.test.ts 8/8, tests/app-router-experimental-react.test.ts 1/1), and vp check on the touched files — all green. The design composes correctly with @vitejs/plugin-rsc's own rsc:react-server-dom-webpack-alias (both pre, and this.resolve(..., {skipSelf:true}) lets the experimental resolveId intercept the rewritten react-server-dom-webpack/* specifier). I verified the runtime details against the installed Next package:
__NEXT_EXPERIMENTAL_REACTis the correct env var Next uses to switch its server runtime (dist/esm/server/route-modules/app-*/module.compiled.js). Setting it viadefineis right.- Next's vendored Flight runtime uses
globalThis.__next_require__(not__webpack_require__), so plugin-rsc's__webpack_require__ → __vite_rsc_require__transform wouldn't touch it. The added__next_require__ → __vite_rsc_require__transform is necessary and the flight-dir prefix check correctly covers thecjs/subdir where those tokens live. - All 28 specifiers resolve (non-null) for rsc/ssr/client against the current Next layout, so the production path is sound.
A few things worth addressing:
1. Dev mode: dep optimizer bypasses the experimental aliases (likely dual-React in dev)
The new aliasing lives only in the main plugin's resolveId. But the dev dep optimizer uses its own Rolldown pipeline that does not see the main plugin hooks — that's exactly why depOptimizeAliasPlugin exists (index.ts:2358, see the comment at index.ts:2354-2357 and #834). The experimental aliases are not registered there, and the RSC env still force-includes react-server-dom-webpack/static.edge in optimizeDeps.include (index.ts:2456). In vp dev with experimental.taint enabled, the prebundled deps would resolve stable React while the source graph resolves experimental React → version mismatch / "Invalid hook call" risk in dev.
The PR only adds a production createBuilder + preview test, so this gap is untested. Either (a) add the experimental aliases to depOptimizeAliasPlugin (and ensure the prebundled static.edge resolves to the experimental copy), or (b) add a dev-mode test for the fixture to confirm dev works. At minimum, please confirm dev behavior — most users hit vp dev first.
2. Non-null assertions will throw an opaque error if Next changes its exports
createExperimentalReactEnvironmentAliases ends with three resolveExperimentalReactSpecifier(...)! (and the hardcoded index.js for the flight runtime). Today every specifier resolves, but if a future Next vendored package drops/renames a condition, these ! turn into a cryptic Cannot read properties of null (reading 'replace') deep in Vite. Given resolveExperimentalReactAliases already throws nice [vinext] ... errors when a package is missing, consider a matching explicit guard here instead of !.
3. react-server-dom-webpack-experimental alias hardcodes index.js
The other two compiled aliases go through resolveExperimentalReactSpecifier (respecting export conditions), but the flight one is hardcoded to path.join(packages["react-server-dom-webpack"], "index.js"). That happens to match the package's "." : "./index.js", but it's an inconsistency that will silently break if the . export ever becomes conditional. Routing it through resolveExperimentalReactSpecifier(packages, "react-server-dom-webpack", environment) would be more robust and consistent.
4. Minor
- The
transformhandler returns a bare string (no sourcemap); plugin-rsc's equivalent returns{ code, map: null }. Harmless but worth matching for consistency / to avoid Vite's missing-sourcemap heuristics. needsExperimentalReactis correctly ignoring the legacyuseExperimentalReactflag from #2167 (the test[{ useExperimentalReact: false, taint: true }, true]documents that). Good — just flagging it's intentional divergence from that PR's config-only knob, consistent with the PR description.
Nothing here blocks the production path, which matches Next v16.2.6 behavior. Item #1 (dev parity) is the one I'd want resolved or explicitly confirmed before merge, per the repo's dev/prod parity guidance in AGENTS.md.
|
To https://github.com/cloudflare/vinext.git |
|
@james-elicx Bonk workflow was cancelled. View workflow run · To retry, trigger Bonk again. |
Performance benchmarksCompared 0 improved · 1 regressed · 5 within ±1.5%
View detailed results and traces 🟢 improvement · 🔴 regression · ⚫ change below 1.5% · paired base/head |
|
/bigbonk review for issues |
|
/bigbonk review for issues |
|
/bigbonk review for issues |
|
/bigbonk review for issues |
There was a problem hiding this comment.
Review: fix(app-router): use experimental React channel
Re-reviewed after the three follow-up commits (align experimental React in dev, resolve experimental React for fixtures, cover experimental React edge cases). I checked out the branch, ran the suites against the installed Next (16.2.7), and verified runtime details against the vendored packages.
Verification
vp test run tests/experimental-react.test.ts— 10/10 ✅vp test run tests/app-router-experimental-react.test.ts— 4/4 ✅ (build+preview, dev optimizer, and bothssr.externalrejection cases)vp checkon all touched files — clean ✅- Confirmed
__NEXT_EXPERIMENTAL_REACTis the correct switch indist/esm/server/route-modules/app-*/module.compiled.js. - Confirmed the vendored Flight runtime uses
globalThis.__next_require__only under thecjs/subdir, so the prefix-scoped transform is correctly targeted, and__vite_rsc_require__is a plugin-rsc global defined in all environments (plugin-rsc's own__webpack_require__rewrite has no env filter), so the new__next_require__rewrite is consistent and safe across rsc/ssr/client.
The four items from the prior review are all resolved:
- Dev parity — addressed via the
vinext:experimental-react-dev-optimizerplugin that rewritesoptimizeDeps.include, registers Rolldown + esbuild resolvers, strips React fromresolve.external, and disablesexternalizeSsrReactInDev. Backed by the new dev-mode test (uses the experimental React channel in the dev dependency optimizer). - Non-null assertions — replaced with
requireExperimentalReactSpecifier, which throws an explicit[vinext] ...error per environment. - Hardcoded
index.js— the Flight alias now routes throughrequireExperimentalReactSpecifier(packages, "react-server-dom-webpack", environment), respecting export conditions. - Sourcemap — the transform now returns
{ code, map: null }, matching plugin-rsc.
Remaining observations (non-blocking):
-
selectPackageExportshort-circuits on the first matching condition without falling back. Inexperimental-react.ts:193-195, once a condition key matches, it recurses and returns even if the nested lookup yieldsnull, instead of continuing to the next condition (e.g.default). This is harmless against the current Next.js layout (all conditional exports are a single flat level), but it diverges from Node's conditional-exports algorithm and would silently mis-resolve if Next ever nests conditions underreact-server. Worth a follow-up or a comment noting the assumption. -
optimizeDeps.includerewriting depends on plugin-rsc'shasReactServerDomWebpackbranch. The dev optimizer maps@vitejs/plugin-rsc/vendor/react-server-dom/*→react-server-dom-webpack/*before alias matching. Today vinext forcesreact-server-dom-webpackintossr.noExternal, so plugin-rsc emits barereact-server-dom-webpack/*specifiers and the replace is a no-op that still matches. The logic correctly covers both forms, but it's an implicit coupling to plugin-rsc internals — fine for now, just fragile if either side changes the vendor naming. -
Replacing
optimizeDeps.includespecifiers with absolute file paths works (the dev test proves it), but is an unusual use ofinclude. No action needed; flagging for awareness.
Nothing blocks merge. Production path matches Next v16.2.6 behavior, and the dev gap raised in the prior review is now closed and tested.
|
Review posted to PR #2372. Summary of findings: Review outcome: no blocking issuesThe PR is solid. I re-reviewed after the three follow-up commits and confirmed all four items from the prior
Verification: all tests pass (10/10 + 4/4), Non-blocking observations flagged:
|
|
/bigbonk review for issues |
|
/bigbonk review for issues |
|
/bigbonk review for issues |
There was a problem hiding this comment.
Review: fix(app-router): use experimental React channel
Re-reviewed after the two commits pushed since the last ask-bonk review (72a6b86 reject external experimental React packages, 577264a preserve stable React dev renderer). The earlier review's "10/10 + 4/4" predates these commits. Running the suite on the current tip surfaces a regression.
🔴 Blocking: tests/app-router-experimental-react.test.ts fails on this branch
Commit 72a6b86 widened the runtime error message in index.ts:1471-1472:
"[vinext] Externalizing React through `ssr.external` or `serverExternalPackages` is incompatible …"
but left the existing assertion in expectExternalReactRejection unchanged (tests/app-router-experimental-react.test.ts:120):
).rejects.toThrow("Externalizing React through `ssr.external` is incompatible");The old substring no longer matches — `ssr.external` is now followed by or `serverExternalPackages`, not is incompatible. Both tests that call this helper now fail:
FAIL tests/app-router-experimental-react.test.ts > rejects ssr.external true …
FAIL tests/app-router-experimental-react.test.ts > rejects React entries in ssr.external
Expected: "Externalizing React through `ssr.external` is incompatible"
Received: "[vinext] Externalizing React through `ssr.external` or `serverExternalPackages` is incompatible …"
Test Files 1 failed (1)
Tests 2 failed | 3 passed (5)
These two cases are not gated behind VINEXT_TEST_NEXT_PACKAGE_DIR (they call createServer directly and reject before any Next resolution), so they run — and fail — in the standard Vitest CI job. This will block the required Vitest check.
Fix: update the assertion at line 120 to a substring that survives the new wording, e.g. "Externalizing React through `ssr.external`" (drop the trailing is incompatible), matching the style of the new serverExternalPackages test at line 145 ("serverExternalPackages is incompatible").
Other observations (non-blocking)
-
No test for the new Pages Router renderer change (
577264a).createPagesRenderToReadableStream(dev-server.ts:92) now selects a statically-importednativeRenderToReadableStreamfor the stable path and only routes through the dev runner whenuseExperimentalReactis true. The rationale is sound (stable dev keeps both the renderer and page components on Node-resolved React viaexternalizeSsrReactInDev), but there is no test exercising either branch. Given AGENTS.md's dev/prod parity emphasis and that this touches every Pages Router dev render + error page, a focused test (stable + experimental) would be worth adding. -
useExperimentalReactis computed for Pages-only apps too, but aliases are App-Router-gated.nextConfig.useExperimentalReactis set unconditionally fromexperimental(next-config.ts:1666), whileexperimentalReactAliasesis only builtif (hasAppDir && …)(index.ts:1461). For a Pages-only app withexperimental.taint,createSSRHandlerreceivesuseExperimentalReact = trueand loadsreact-dom/server.edgevia the runner, but no aliasing is active so it resolves stable React — which happens to be consistent (becauseexternalizeSsrReactInDevis also true whenexperimentalReactAliasesis null). It works today, but the coupling is implicit; a comment or a Pages-aware gate would make the invariant explicit. -
Duplicated specifier-matching after the
isExperimentalReactSpecifierextraction. Commit72a6b86extracted theEXPERIMENTAL_REACT_SPECIFIERS.some(...)pattern intoisExperimentalReactSpecifier, but the dev optimizer'sresolve.externalfilter atindex.ts:5793-5795still inlines the same logic. Reusing the helper there would keep the two in sync. -
selectPackageExportfirst-match-wins (experimental-react.ts:199-202) returns once a condition key is present even if the nested lookup isnull, rather than continuing todefault. Confirmed harmless against the current vendored layout (all conditional exports — e.g.react-dom/server.edge→{ "react-server", "default" }— are a single flat level), but it diverges from Node's resolution and would mis-resolve under nested conditions. Worth a comment noting the assumption.
The production design remains sound and matches Next v16.2.6. The single blocking item is the stale error-message assertion — once line 120 is updated, the suite is green.
|
Review posted to PR #2372. Summary of findings🔴 One blocking issue — a test regression that will fail CI's Vitest check: Commit I reproduced this by running the suite on the current branch tip. The prior Fix: trim the assertion to Non-blocking observations:
The production design is sound and matches Next v16.2.6; the suite goes green once line 120 is updated. |
|
/bigbonk review for issues |
There was a problem hiding this comment.
Review: fix(app-router): use experimental React channel
Re-reviewed at branch tip (85932ec), which adds two commits since the last ask-bonk review: fbbdb9a match experimental React external error and 85932ec gate experimental renderer on aliases. I checked out the branch, ran the affected suites against the installed Next (16.2.7), type-checked the package, and traced the dev renderer / aliasing invariants.
✅ The prior blocking issue is resolved
The stale assertion at tests/app-router-experimental-react.test.ts:120 is fixed (commit fbbdb9a). It now asserts the substring "Externalizing React through `ssr.external` or `serverExternalPackages`", which matches the widened runtime error at index.ts:1471-1473. Both previously-failing ungated rejection cases now pass.
Verification (all green):
tests/app-router-experimental-react.test.ts— 5/5 (build+preview, dev optimizer, bothssr.externalrejection cases,serverExternalPackagesrejection)tests/experimental-react.test.ts+tests/next-config.test.ts— 219/219tests/pages-router.test.ts— 318/318 (stable Pages renderer path intact)tests/app-router-dev-server.test.tsoptimizeDeps test — passtsc --noEmiton the vinext package — clean
✅ Prior observation #2 (Pages renderer coupling) is now addressed
Commit 85932ec changes the dev Pages SSR renderer gate at index.ts:4468 from nextConfig?.useExperimentalReact to experimentalReactAliases !== null. This is the correct fix and makes the previously-implicit invariant explicit:
experimentalReactAliases !== null(App Router + experimental flag):createPagesRenderToReadableStreamreturns the runner-loadedreact-dom/server.edge(aliased → experimental), andexternalizeSsrReactInDevisfalse(index.ts:2358-2362), so page components also resolve aliased experimental React. Consistent.experimentalReactAliases === null(everything else, incl. a Pages-only app withexperimental.taint): renderer returns the statically-importednativeRenderToReadableStream(Node-stable), andexternalizeSsrReactInDevistrue, so page components are Node-externalized stable React. Consistent.
Both the renderer and the page tree now key off the same condition, so the Pages-only-app divergence flagged previously can no longer occur. createSSRHandler is dev-only (single call site at index.ts:4453); the production Pages path lives in prod-server.ts/worker-entry.ts and bakes the aliases at build time, so there's no parity gap to mirror.
Non-blocking observations (carried forward, unchanged)
-
Duplicated specifier-matching. The dev optimizer's
resolve.externalfilter still inlinesEXPERIMENTAL_REACT_SPECIFIERS.some(...)atindex.ts:5793-5795instead of reusing the extractedisExperimentalReactSpecifierhelper. Minor; reusing it keeps the two in sync. -
selectPackageExportfirst-match-wins (experimental-react.ts:199-202) returns once a condition key is present even if the nested lookup resolves tonull, rather than continuing todefault. Harmless against the current flat vendored exports, but diverges from Node's conditional-exports algorithm and would mis-resolve under nested conditions. Worth a comment noting the assumption. -
optimizeDeps.includerewrite still couples implicitly to plugin-rsc'shasReactServerDomWebpackvendor naming (handles both@vitejs/plugin-rsc/vendor/react-server-dom/*and barereact-server-dom-webpack/*). Fine today; fragile if either side changes the naming.
Outcome: no blocking issues
The blocking test regression from the last review is fixed and the suite is green. The production design matches Next v16.2.6, and the dev renderer is now gated on the actual aliasing state. The remaining items are all non-blocking follow-ups.
|
Review posted to PR #2372. SummaryI re-reviewed PR #2372 at the branch tip ( Outcome: no blocking issues. The blocking test regression flagged in the prior review is resolved:
The prior non-blocking observation #2 (implicit Pages-router coupling) is also addressed:
Verification (all green): experimental-react (5/5), unit + next-config (219/219), pages-router (318/318), the dev-server optimizeDeps test, and a clean Remaining items are non-blocking follow-ups: duplicated specifier-matching at |
|
/bigbonk review for issues |
|
/bigbonk review for issues |
|
/bigbonk review for issues |
|
/bigbonk review for issues |
There was a problem hiding this comment.
Review: fix(app-router): use experimental React channel
Re-reviewed at branch tip (737aaec), which adds three commits since the last ask-bonk review: dd68992 interop experimental React in dev, 03d23d5 allow React dev eval in CSP fixture, 737aaec align dynamic CSP expectation. The prior "no blocking issues" review predates all three. Running the affected suite on the current tip surfaces a regression.
🔴 Blocking: tests/nextjs-compat/use-client-page-pathname.test.ts fails on this branch
Commit 03d23d5 widened the CSP header that app-basic's shared middleware sets for the /use-client-page-pathname?csp-nonce branch (tests/fixtures/app-basic/middleware.ts:293-296):
r.headers.set(
"content-security-policy",
"script-src 'nonce-vinext-test-nonce' 'strict-dynamic' 'unsafe-eval';",
);The commit updated the two assertions that exercise this fixture in tests/e2e/.../csp.spec.ts and tests/nextjs-compat/dynamic.test.ts, but missed the assertion at tests/nextjs-compat/use-client-page-pathname.test.ts:107-108, which still expects the pre-change strict value:
const res = await fetch(`${_baseUrl}${ROUTE}?csp-nonce=1`); // ROUTE = "/use-client-page-pathname"
expect(res.headers.get("content-security-policy")).toBe(
"script-src 'nonce-vinext-test-nonce' 'strict-dynamic';", // <- no 'unsafe-eval'
);Since the middleware now emits 'unsafe-eval' for exactly this path+query, the assertion no longer matches. I reproduced it on the current tip:
FAIL tests/nextjs-compat/use-client-page-pathname.test.ts > adds CSP nonce to inline hydration and bootstrap scripts
Expected: "script-src 'nonce-vinext-test-nonce' 'strict-dynamic';"
Received: "script-src 'nonce-vinext-test-nonce' 'strict-dynamic' 'unsafe-eval';"
Test Files 1 failed (1)
Tests 1 failed | 10 skipped (11)
This test is not gated behind VINEXT_TEST_NEXT_PACKAGE_DIR — it hits the app-basic dev fixture directly and runs in the standard Vitest CI job. This will block the required Vitest check.
Fix: update line 108 to include 'unsafe-eval', matching the sibling assertions updated in dynamic.test.ts:147 and csp.spec.ts:
"script-src 'nonce-vinext-test-nonce' 'strict-dynamic' 'unsafe-eval';",The other tests that hit this fixture without ?csp-nonce (use-client-page-pathname.test.ts:138/:149, script-nonce.test.ts) are unaffected and pass — confirmed locally.
Worth flagging: why the dev CSP now needs 'unsafe-eval'
The root cause of the CSP loosening is that app-basic's next.config.ts has experimental.gestureTransition: true, so needsExperimentalReact() is true and the whole fixture now runs on the experimental React channel. In vp dev, commit dd68992 resolves the client React alias to the vendored *.development.js CJS build and runs it through a second commonjs() pass; serving that under Vite dev requires 'unsafe-eval', hence the fixture/CSP changes.
This is a real, user-visible behavioral change: any app using the experimental React channel (taint / transitionIndicator / gestureTransition) will need 'unsafe-eval' in its dev CSP. Two things to confirm before merge:
- This is dev-only. The production CSP assertions in
tests/app-router-production-server.test.tsare unchanged and still strict — good — but please confirm the production experimental build does not pull in an eval-using dev bundle. - Whether this matches Next.js dev behavior. Next.js dev also injects an
'unsafe-eval'allowance for React Refresh, so this is likely parity-correct, but it's worth a one-line note in the PR since it changes the dev security posture for experimental-channel apps.
Non-blocking observations (carried forward)
resolveExperimentalReactDevelopmentEntryregex parsing is fragile (experimental-react.ts:99-105). It scrapesmodule.exports = require('./cjs/*.development.js')out of the NODE_ENV wrapper. It correctly falls back to the original entry on no-match (so non-wrapper exports are safe), but it silently depends on the exactif (production) … else { module.exports = require(...) }shape of Next's vendored wrapper. A comment noting the assumption (and that the fallback is intentional) would help.selectPackageExportfirst-match-wins (experimental-react.ts:207-216) — unchanged from prior reviews; harmless against the current flat vendored exports but diverges from Node's conditional-exports resolution.- Duplicated specifier-matching at
index.ts:5793still inlinesEXPERIMENTAL_REACT_SPECIFIERS.some(...)instead of reusingisExperimentalReactSpecifier.
The production design remains sound and matches Next v16.2.6. The single blocking item is the stale CSP assertion at use-client-page-pathname.test.ts:108 — once that line is updated the suite goes green.
|
Review posted to PR #2372. SummaryI re-reviewed PR #2372 at the branch tip ( 🔴 One blocking issue — a test regression that will fail CI's Vitest check: Commit This test is ungated (runs against the Also flagged (worth confirming, not strictly blocking): the root cause is that Non-blocking: fragile regex in The production design is sound and matches Next v16.2.6; the suite goes green once line 108 is updated. |
|
/bigbonk review for issues |
There was a problem hiding this comment.
Review: fix(app-router): use experimental React channel
Re-reviewed at branch tip (d39f2bc), which adds two commits since the last ask-bonk review: 4537fd4 align navigation CSP expectation and d39f2bc scope unsafe eval CSP to dev. The prior review's blocking item (stale CSP assertion at use-client-page-pathname.test.ts:108) is now resolved — and the fix is better than the one I previously suggested.
✅ The prior blocking issue is resolved (and improved)
Rather than just appending 'unsafe-eval' to the test assertion unconditionally, the author scoped the fixture middleware's 'unsafe-eval' to dev only (d39f2bc, tests/fixtures/app-basic/middleware.ts:293,331):
const devEvalSource = process.env.NODE_ENV === "development" ? " 'unsafe-eval'" : "";
r.headers.set(
"content-security-policy",
`script-src 'nonce-vinext-test-nonce' 'strict-dynamic'${devEvalSource};`,
);This keeps the production CSP strict while allowing the eval-using dev bundle, which is the correct behavioral truth: experimental React's dev CJS bundle needs 'unsafe-eval', but the production build does not. The matching dev assertion at use-client-page-pathname.test.ts:108 (4537fd4) now expects the dev value.
Verification (all green)
tests/nextjs-compat/use-client-page-pathname.test.ts— 11/11 (the previously-failingcsp-noncecase now passes; dev server setsNODE_ENV=development)tests/nextjs-compat/dynamic.test.ts— 12/12 (dev, expects'unsafe-eval')tests/experimental-react.test.ts— 11/11tests/app-router-experimental-react.test.ts— 5/5 (build+preview, dev optimizer, both rejection cases)tests/app-router-production-server.test.tsCSP-nonce cases — 6/6, all strict ('nonce-… 'strict-dynamic';with NO'unsafe-eval'), against a real production build of theapp-basicfixture (which hasexperimental.gestureTransition: true, so the whole fixture runs on the experimental channel)vp checkon all touched files — clean
This directly answers the two confirmation requests from the prior review:
- The change is dev-only. Production CSP assertions remain strict and pass against the experimental-channel production build — confirming the production build does not pull in an eval-using bundle. ✅
- The DEV E2E project (
csp.spec.ts) runsvp dev(documented atcsp.spec.ts:13), so its'unsafe-eval'assertions are correct; the production nonce-on-preload assertions live in the prod-server Vitest suite, which stays strict. ✅
Worth flagging (non-blocking)
- The fixture middleware now keys CSP on
process.env.NODE_ENV. This works because Vite'screateServersetsNODE_ENV=development(dev tests), while the prod-server build/Vitest worker does not. It's empirically validated by both suites passing, but it's an implicit coupling to Vite's env mutation. It's test-fixture code, not production logic, so this is fine — just noting the mechanism. - User-visible dev behavior change: any app using the experimental React channel (
taint/transitionIndicator/gestureTransition) will need'unsafe-eval'in its dev CSP. This matches Next.js dev (which already allows'unsafe-eval'for React Refresh). A one-line note in the PR description would help future readers.
Non-blocking observations (carried forward, unchanged)
- Duplicated specifier-matching at
index.ts:5809-5811still inlinesEXPERIMENTAL_REACT_SPECIFIERS.some(...)instead of reusing the extractedisExperimentalReactSpecifierhelper. Minor; reusing it keeps the two in sync. selectPackageExportfirst-match-wins (experimental-react.ts:212-215): once a condition key is present it recurses and returns even if the nested lookup isnull, rather than continuing todefault. Harmless against the current flat vendored exports, but diverges from Node's conditional-exports resolution and would mis-resolve under nested conditions. Worth a comment noting the assumption.resolveExperimentalReactDevelopmentEntryregex (experimental-react.ts:101-104) scrapes theNODE_ENVwrapper; it safely falls back to the original entry on no-match but silently depends on Next's vendored wrapper shape. A comment noting the intentional fallback would help.
Outcome: no blocking issues
The blocking CSP-assertion regression from the last review is fixed, and the fix correctly scopes 'unsafe-eval' to dev while keeping production strict. The full affected suite is green, the production design matches Next v16.2.6, and dev/prod parity is preserved. The remaining items are non-blocking follow-ups.
|
Review posted to PR #2372. SummaryI re-reviewed PR #2372 at branch tip ( Outcome: no blocking issues. The prior blocking issue — the stale CSP assertion at Verification (all green, run locally):
This confirms the two open questions from the prior review: the change is dev-only, and the production experimental build does not pull in an eval-using bundle. Remaining items are non-blocking follow-ups (duplicated specifier-matching at |
|
/bigbonk review for issues |
There was a problem hiding this comment.
Review: fix(app-router): use experimental React channel
Re-reviewed at branch tip (85cdd07), which adds one commit since the last ask-bonk review: 85cdd07 fix(pages-router): preserve experimental React channel. The prior "no blocking issues" verdict predates it.
Outcome: no blocking issues
The new commit is a correctness fix, and it closes the implicit-coupling gap flagged as non-blocking observation #2 in several prior reviews.
What changed: the experimental-React alias setup at index.ts:1477 is no longer gated on hasAppDir — it now fires whenever nextConfig.useExperimentalReact is true:
- if (hasAppDir && nextConfig.useExperimentalReact) {
+ if (nextConfig.useExperimentalReact) {Why this is correct (and a real fix): before this commit, a Pages-only app (or a hybrid app with disableAppRouter: true) that set experimental.taint / transitionIndicator / gestureTransition would compute useExperimentalReact = true, but experimentalReactAliases stayed null. Two consequences followed silently:
- The
process.env.__NEXT_EXPERIMENTAL_REACTdefine is gated onexperimentalReactAliases(index.ts:1554), so it was never set — the Next server runtime stayed on the stable channel. - No React aliasing was active, so page components resolved stable React despite the user opting into the experimental channel.
That is the exact divergence prior reviews called out. This commit makes the Pages SSR build pick up the experimental channel, and I confirmed the rest of the machinery is already environment-gated rather than hasAppDir-gated, so nothing else needs to change for parity:
vinext:experimental-react-channelresolveId/transformgate only onexperimentalReactAliases+ environment name (index.ts:1205-1237).vinext:experimental-react-dev-optimizerconfigEnvironmentgates onexperimentalReactAliases+rsc/ssr/client(index.ts:5795-5798).externalizeSsrReactInDevis!experimentalReactAliases(index.ts:2374-2378), and the dev Pages renderer gate isexperimentalReactAliases !== null(index.ts:4484). Both now flip together for a Pages-only experimental app, so renderer and page-tree React channels stay consistent — the same invariant85932ecestablished for the App Router case.
The Pages production SSR build runs in the default ssr environment, which the channel resolveId matches, so the production Pages path is aliased too. The always-created rsc aliases (index.ts:1493) are inert for a Pages-only app (no rsc environment), so no harm.
New test is adequate: keeps the experimental React channel in router-disabled hybrid Pages builds resolves config with disableAppRouter: true and asserts both process.env.__NEXT_EXPERIMENTAL_REACT === "true" and that the ssr-environment react specifier resolves into /next/dist/compiled/react-experimental/. That directly covers the previously-broken path.
Verification (all green, run locally against installed Next 16.2.7)
tests/app-router-experimental-react.test.ts— 6/6 (build+preview, dev optimizer, bothssr.externalrejection cases,serverExternalPackagesrejection, and the new router-disabled hybrid case)tests/experimental-react.test.ts— 11/11tests/pages-router.test.ts— 318/318 (stable Pages renderer path intact)tests/nextjs-compat/use-client-page-pathname.test.ts— 11/11tests/nextjs-compat/dynamic.test.ts— 12/12tests/app-router-dev-server.test.ts+tests/next-config.test.ts— passvp checkonindex.ts,experimental-react.ts, and the test — clean
Non-blocking observations (carried forward, unchanged)
- Duplicated specifier-matching at
index.ts:5809-5811still inlinesEXPERIMENTAL_REACT_SPECIFIERS.some(...)instead of reusing the extractedisExperimentalReactSpecifierhelper. Minor; reusing it keeps the two in sync. selectPackageExportfirst-match-wins (experimental-react.ts:212-215): once a condition key is present it recurses and returns even if the nested lookup isnull, rather than continuing todefault. Harmless against the current flat vendored exports, but diverges from Node's conditional-exports resolution and would mis-resolve under nested conditions. Worth a comment noting the assumption.resolveExperimentalReactDevelopmentEntryregex (experimental-react.ts:101-104) scrapes theNODE_ENVwrapper; it safely falls back to the original entry on no-match but silently depends on Next's vendored wrapper shape. A comment noting the intentional fallback would help.- Dev
'unsafe-eval'CSP for experimental-channel apps (from earlier commits) is now also reachable by Pages-only experimental apps in dev, since the experimental dev CJS bundle is loaded there too. Production stays strict. Matches Next.js dev (React Refresh already allows'unsafe-eval'); a one-line PR note would help future readers.
Conclusion
No blocking issues. The latest commit is a sound correctness fix that aligns Pages-only / router-disabled experimental apps with the App Router behavior, and it's covered by a focused test. Production design matches Next v16.2.6 and dev/prod parity is preserved.
|
Review posted to PR #2372. SummaryI re-reviewed PR #2372 at branch tip ( Outcome: no blocking issues. The new commit removes the
Verification (all green, against installed Next 16.2.7):
Remaining items are all non-blocking follow-ups (duplicated specifier-matching, |
|
Closing this attempt after reverting it to a zero-diff branch. The fixture-wide A safe follow-up needs to make vinext's client navigation/history runtime compatible with the experimental React channel before enabling it globally; keeping a mixed stable-client/experimental-server channel would create hydration/runtime mismatches, so that workaround is not acceptable. |
1 participant
Summary
experimental.taint,experimental.transitionIndicator, orexperimental.gestureTransitionis enabledNext.js parity
Matches
test/e2e/app-dir/rsc-basic/rsc-basic-react-experimental.test.tsfrom Next.js v16.2.6.Validation
vp check packages/vinext/src/config/experimental-react.ts packages/vinext/src/config/next-config.ts packages/vinext/src/index.ts tests/experimental-react.test.ts tests/app-router-experimental-react.test.ts tests/fixtures/app-experimental-react/app/client-react.tsxVINEXT_TEST_NEXT_PACKAGE_DIR=... vp test run tests/experimental-react.test.ts tests/app-router-experimental-react.test.ts --maxWorkers=1— 9/9 passedOverlap
PR #2167 accepts an explicit config option but intentionally does not implement runtime channel parity. This PR follows Next.js v16.2.6's actual automatic selection and runtime behavior.