Shim next/root-params#941
Conversation
commit: |
|
/bigbonk review |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: Shim next/root-params
Clean first contribution — the scanner, shim, generated entry wiring, and tests all hang together well. The overall design (scan root layout params at build time, generate a virtual module with named exports, set per-request state in the RSC entry) matches the Next.js approach.
A few issues to address before merging:
Must fix
-
Root params state is never cleaned up on error/early-return paths.
setNavigationContext(null)is called in ~30 places throughout the generated entry for cleanup on error, early return, not-found, redirect, etc.__setRootParamsis only set once and never nulled out. When requests run outside the unified ALS scope (SSR environment, standalone ALS fallback), this leaks state between requests. This needs parity with thesetNavigationContext(null)cleanup pattern. -
RESOLVED_ROOT_PARAMSvirtual module is not invalidated on file changes in dev. When a new layout or dynamic segment is added during development, the file watcher invalidates the app route cache and the RSC entry module (lines 2048–2051 ofindex.ts), but does not invalidate theRESOLVED_ROOT_PARAMSmodule. Thenext/root-paramsvirtual module will serve stale exports until a full dev server restart. Add aninvalidateRootParamsModule()call alongsideinvalidateRscEntryModule()in both theaddandunlinkwatcher handlers.
Should fix
-
Synthetic parallel-slot routes don't set
rootParamNames.synthesizeParallelSlotRoutes(around line 422 ofapp-router.ts) builds new route objects that inherit most fields from the parent route but don't copyrootParamNames. The field is optional and the entry template handlesundefinedvia?? [], so this won't crash, but it means parallel-slot routes silently lose their root params. Should propagaterootParamNames: parentRoute.rootParamNamesin the synthetic route. -
runWithRootParamsContextis exported but never used. The other shims follow a pattern whererunWith*Contextwraps the per-request scope. If the unified request context is always used in the RSC entry (which it is), this function is dead code. Either use it or remove it to keep the API surface minimal.
Nits
- See inline comments below.
| searchParams: url.searchParams, | ||
| params, | ||
| }); | ||
| __setRootParams(__pickRootParams(params, route.rootParamNames)); |
There was a problem hiding this comment.
State leak: __setRootParams is called here but never cleaned up on error/early-return paths. Compare with setNavigationContext(null) which is called in ~30 places throughout this file for cleanup. Every path that calls setNavigationContext(null) should also call __setRootParams(null) to prevent leaking root params state to the next request when running outside the unified ALS scope.
At minimum, you need cleanup on:
- All early-return/error paths after this line (route handler errors, page render errors, ISR fallback paths, etc.)
- The final cleanup at the end of
_handleRequest
The unified request context handles this for the common path (since the whole context is scoped to the ALS run), but the standalone ALS fallback in root-params.ts would leak.
There was a problem hiding this comment.
Fixed in 68a8755. Root params now clear through generated __clearRequestContext(), and clearing navigation context also clears root params so early-return/error paths do not leak fallback state.
| instrumentationPath, | ||
| ); | ||
| } | ||
| if (id === RESOLVED_ROOT_PARAMS) { |
There was a problem hiding this comment.
HMR gap: This virtual module is generated at load() time based on the current routes, but it's never invalidated when the file watcher detects new/removed files. The RSC entry gets invalidated at line 2050 (invalidateRscEntryModule()), but the root-params module doesn't.
When a developer adds app/[lang]/layout.tsx during a dev session, the next/root-params module will still serve the old (empty) exports until a full restart.
Add invalidation alongside the RSC entry invalidation:
function invalidateRootParamsModule() {
for (const env of Object.values(server.environments)) {
const mod = env.moduleGraph.getModuleById(RESOLVED_ROOT_PARAMS);
if (mod) env.moduleGraph.invalidateModule(mod);
}
}Then call it in the add/unlink watcher handlers.
There was a problem hiding this comment.
Fixed in 68a8755. App route add/unlink watcher handling now invalidates the root params virtual module across Vite environments alongside the RSC entry.
| layoutTreePositions, | ||
| isDynamic, | ||
| params, | ||
| rootParamNames: computeRootParamNames(segments, layoutTreePositions), |
There was a problem hiding this comment.
Minor: computeRootParamNames is only relevant when the first layout is NOT at the app root (position > 0). That's the uncommon case — most apps have app/layout.tsx at position 0 and get an empty array. The logic is correct, just noting this is a niche feature that primarily applies to i18n patterns like app/[lang]/layout.tsx.
| return _als.getStore() ?? _fallbackState; | ||
| } | ||
|
|
||
| export function runWithRootParamsContext<T>(fn: () => Promise<T>): Promise<T>; |
There was a problem hiding this comment.
runWithRootParamsContext is exported but never imported or called anywhere in the codebase. The RSC entry uses the unified request context (which already includes rootParams: null via createRequestContext), and the standalone ALS is only used as a fallback.
Suggest removing it to keep the API surface minimal, or if you intend to use it for SSR-side isolation, wire it up.
There was a problem hiding this comment.
Fixed in 68a8755. Removed the unused runWithRootParamsContext export and kept root params scoped through the unified request context fallback path.
| return /^[$A-Z_a-z][$\w]*$/.test(name); | ||
| } | ||
|
|
||
| function generateRootParamsModule(rootParamNames: Iterable<string>): string { |
There was a problem hiding this comment.
Nit: the generated module creates top-level named function exports. Next.js does the same thing (the placeholder next/root-params.js is compiler-replaced with getter calls). This is a good match.
One thing to consider: Next.js gates rootParams behind experimental.rootParams: true (or cacheComponents: true). This implementation is always-on, which is fine for now but worth documenting as a known divergence.
| return Object.assign(Promise.resolve(plain), plain); | ||
| } | ||
|
|
||
| function __pickRootParams(params, rootParamNames) { |
There was a problem hiding this comment.
Per the AGENTS.md guidance about keeping generated entries thin: __pickRootParams is a small utility, but it's the kind of logic that could live in the runtime root-params.ts module and be imported. Currently it's inlined in the generated template string.
Not blocking, but if you're doing a follow-up pass, consider moving this to shims/root-params.ts as an exported pickRootParams function and importing it in the entry.
There was a problem hiding this comment.
Fixed in 68a8755. Root param selection moved out of generated entry code into the runtime pickRootParams shim, with generated-code and runtime tests covering the delegation.
|
Review submitted. Here's a summary of the findings: Must fix (2):
Should fix (2): Nits: Inline suggestions about moving |
|
/bigbonk review |
| if (ctx === null) __setRootParams(null); | ||
| } | ||
|
|
||
| function __clearRequestContext() { |
There was a problem hiding this comment.
i kid you not, i was thinking about how something like this would be much nicer about an hour ago. nice one!
There was a problem hiding this comment.
Review: Shim next/root-params (post-fixup)
The previous review's four issues have all been addressed in 68a8755:
-
State leak (fixed) —
__clearRequestContext()now consolidates all cleanup (setHeadersContext,setNavigationContext,__setRootParams) into a single function, and every early-return/error path uses it. ThesetNavigationContext(null)wrapper also clears root params as a belt-and-suspenders measure. -
HMR invalidation (fixed) —
invalidateRootParamsModule()is called in bothaddandunlinkwatcher handlers alongsideinvalidateRscEntryModule(). -
Parallel-slot routes (fixed) —
synthesizeParallelSlotRoutesnow propagatesrootParamNames: parentRoute.rootParamNames, with a dedicated test ("preserves root layout params on synthetic parallel slot routes"). -
Dead code (fixed) —
runWithRootParamsContextremoved. -
pickRootParamsmoved to runtime (fixed) — Extracted from the generated entry template toshims/root-params.ts, imported by the entry.
The overall approach is solid:
- Scanner computes root param names at build time based on segments before the first layout
- Virtual module (
next/root-params) generates per-param named exports usinggetRootParam - Per-request state flows through the unified ALS context with a global fallback
- Test coverage is good: scanner unit tests, generated-entry assertions, runtime getter test, and parallel-slot propagation
One minor observation below, but nothing blocking.
| function __clearRequestContext() { | ||
| setHeadersContext(null); | ||
| setNavigationContext(null); | ||
| __setRootParams(null); |
There was a problem hiding this comment.
Nit: __setRootParams(null) is called twice when __clearRequestContext() runs — once via setNavigationContext(null) (line 473) and once directly (line 479). This is harmless (idempotent set to null), but it's slightly misleading to read. If the intent is that setNavigationContext(null) always clears root params as a side effect, the explicit call on line 479 is redundant. If the intent is defense-in-depth (in case someone later removes the side-effect from the wrapper), then a comment would help future readers understand why.
Not blocking — just noting the redundancy.
|
Review submitted. All four issues from the previous review have been addressed. The PR is approved with one minor nit about the redundant |
2 participants
Summary
next/root-paramsruntime exports based on App Router root layout params.Fixes #806