fix: always mark pages that receive searchParams as dynamic#822
fix: always mark pages that receive searchParams as dynamic#822southpolesteve wants to merge 1 commit intomainfrom
Conversation
Pages that read searchParams are request-dependent even when the first request has an empty query string. Previously, markDynamicUsage() was only called when the URL contained query parameters (hasSearchParams), so an empty-query first request would be cached and replayed to later requests with different query parameters. Remove the hasSearchParams guard so markDynamicUsage() is called unconditionally whenever searchParams is provided to the page. This matches Next.js behavior where accessing the searchParams prop signals dynamic rendering regardless of whether the URL has query params. Adds a production integration test with a dedicated fixture page that reads searchParams, verifying the empty-query response is not cached and later query-specific requests see their own parameters.
|
/bigbonk review this PR |
commit: |
|
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
Good fix — the cache-poisoning bug when the first request has an empty query string is a real correctness issue, and the test exactly reproduces the scenario. The change is narrow and the reasoning in the updated comment block is clear.
One minor cleanup and one question:
-
Dead code:
hasSearchParamsvariable — After removing theif (hasSearchParams)guard, thelet hasSearchParams = false(line 983) andhasSearchParams = true(line 986) inside thesearchParams.forEachloop are now unused. They should be removed to avoid confusing readers into thinking it's still referenced somewhere. -
ISR background regen path — The
renderFreshPageForCachecallback at line 2200 callsbuildPageElements(route, params, cleanPathname, undefined, new URLSearchParams(), ...). With this change,markDynamicUsage()will now fire during background regeneration too (sincesearchParamsisnew URLSearchParams(), which is truthy). This is probably fine because:- The regen path already has its own unified context scope
- The
consumeDynamicUsage()inscheduleAppPageRscCacheWritewill detect it and skip the cache write - But that means pages with
searchParams+ ISRrevalidatewill never actually cache — the ISR cache read at line 2164 will always miss, and the write will always be skipped
This is the correct behavior for the bug fix (searchParams pages shouldn't be cached at all), but it does mean the ISR cache read/write machinery runs for nothing on every request to these pages. Not a correctness issue, just wasted work. Worth noting for a future optimization where you could skip the ISR cache read entirely when the route receives searchParams.
The test fixture and assertions look solid. The export const revalidate = 60 on the fixture page is a good choice — it ensures the ISR path is actually engaged, which is what makes the bug manifest.
|
The inline comment API requires diff positions. The review-level comment I already posted covers this feedback adequately, so the review is complete. Review posted on PR #822. Here's a summary of the feedback:
|
| // We cannot use a Proxy here because React's RSC debug serializer accesses | ||
| // properties on all props ($$typeof, etc.), causing false positives. | ||
| // Instead, we unconditionally mark dynamic: any page that receives | ||
| // searchParams is request-dependent, even when the first request has an | ||
| // empty query string. Without this, the empty-query render would be cached | ||
| // and replayed to later requests with different query parameters. |
There was a problem hiding this comment.
Would it be possible to filter out those internals in a proxy instead? If so, we could use the proxy approach and align behaviour with Next.js.
This change as-is would mark every route we give search params to as dynamic, regardles of whether they use search params or not.
2 participants
Summary
Pages that read
searchParamsare now always marked dynamic, even when the first request has an empty query string.Details
Previously,
markDynamicUsage()was only called when the URL contained query parameters (hasSearchParams). A request to/page(no query) would render withsearchParams = {}, not be marked dynamic, and get ISR-cached. A later request to/page?filter=violetwould then receive the cached empty-query response.The fix removes the
hasSearchParamsguard somarkDynamicUsage()is called unconditionally wheneversearchParamsis provided to the page component. This matches Next.js behavior where accessing thesearchParamsprop signals dynamic rendering regardless of whether the URL has query parameters.Tests
Adds a production integration test with a dedicated fixture page (
/search-params-page) that readssearchParams:filter=none?filter=violet: not cached, rendersfilter=violet