-
Notifications
You must be signed in to change notification settings - Fork 306
fix: always mark pages that receive searchParams as dynamic #822
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1140,7 +1140,7 @@ | |
| expect(res.status).toBe(200); | ||
| const cacheControl = res.headers.get("cache-control"); | ||
| // Normal pages should not have no-store | ||
| expect(cacheControl).toBeNull(); | ||
|
Check failure on line 1143 in tests/app-router.test.ts
|
||
| }); | ||
|
|
||
| it("export const dynamic = 'force-static' sets long-lived Cache-Control", async () => { | ||
|
|
@@ -1247,7 +1247,7 @@ | |
|
|
||
| // revalidate=60 should set s-maxage=60 on first request (cache MISS) | ||
| const cacheControl = res.headers.get("cache-control"); | ||
| expect(cacheControl).toContain("s-maxage=60"); | ||
|
Check failure on line 1250 in tests/app-router.test.ts
|
||
| expect(cacheControl).toContain("stale-while-revalidate"); | ||
| }); | ||
|
|
||
|
|
@@ -1968,7 +1968,7 @@ | |
| expect(res2.status).toBe(200); | ||
| const html2 = await res2.text(); | ||
| const reqId2 = extractRequestId(html2); | ||
| expect(reqId2).toBe(reqId1); | ||
|
Check failure on line 1971 in tests/app-router.test.ts
|
||
| expect(res2.headers.get("x-vinext-cache")).toBe("HIT"); | ||
|
|
||
| const tagRes = await fetch(`${baseUrl}/api/revalidate-tag`); | ||
|
|
@@ -1995,7 +1995,7 @@ | |
| expect(res2.status).toBe(200); | ||
| const html2 = await res2.text(); | ||
| const reqId2 = extractRequestId(html2); | ||
| expect(reqId2).toBe(reqId1); | ||
|
Check failure on line 1998 in tests/app-router.test.ts
|
||
| expect(res2.headers.get("x-vinext-cache")).toBe("HIT"); | ||
|
|
||
| const pathRes = await fetch(`${baseUrl}/api/revalidate-path`); | ||
|
|
@@ -2046,6 +2046,26 @@ | |
| expect(html2).not.toContain('"filter">alpha<'); | ||
| }); | ||
|
|
||
| it("page ISR + searchParams: empty-query first request does not cache and poison later query requests", async () => { | ||
| // First request with NO query params. The page reads searchParams, so it | ||
| // must NOT be cached even though the query string is empty. | ||
| const res1 = await fetch(`${baseUrl}/search-params-page`); | ||
| expect(res1.status).toBe(200); | ||
| expect(res1.headers.get("x-vinext-cache")).toBeNull(); | ||
| const html1 = await res1.text(); | ||
| // React inserts <!-- --> between text nodes, so match the id + content pattern | ||
| expect(html1).toContain('id="filter"'); | ||
| expect(html1).toContain("none"); | ||
|
|
||
| // Second request WITH query params must see its own searchParams, not | ||
| // a cached empty-query response. | ||
| const res2 = await fetch(`${baseUrl}/search-params-page?filter=violet`); | ||
| expect(res2.status).toBe(200); | ||
| expect(res2.headers.get("x-vinext-cache")).toBeNull(); | ||
| const html2 = await res2.text(); | ||
| expect(html2).toContain("violet"); | ||
| }); | ||
|
|
||
| // Route handler ISR caching tests | ||
| // These tests are ORDER-DEPENDENT: they share a single production server and | ||
| // /api/static-data cache state persists across tests. HIT depends on MISS | ||
|
|
@@ -2364,7 +2384,7 @@ | |
| loadingPath: null, | ||
| errorPath: null, | ||
| layoutErrorPaths: [], | ||
| notFoundPath: null, | ||
|
Check failure on line 2387 in tests/app-router.test.ts
|
||
| notFoundPaths: [], | ||
| forbiddenPath: null, | ||
| unauthorizedPath: null, | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,16 @@ | ||
| export const revalidate = 60; | ||
|
|
||
| export default async function SearchParamsPage(props: { | ||
| searchParams: Promise<Record<string, string | string[] | undefined>>; | ||
| }) { | ||
| const sp = await props.searchParams; | ||
| const filter = sp.filter ?? "none"; | ||
|
|
||
| return ( | ||
| <div> | ||
| <h1>Search Params Page</h1> | ||
| <p id="filter">filter={String(filter)}</p> | ||
| <p id="keys">keys={Object.keys(sp).sort().join(",")}</p> | ||
| </div> | ||
| ); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.