fix: emit no-store Cache-Control for revalidate = 0 pages#777
Conversation
Pages exporting revalidate = 0 should be treated as fully dynamic (never cached), matching Next.js behavior. The previous code used truthiness checks on revalidateSeconds, causing 0 to fall through with no Cache-Control header — allowing CDNs to cache dynamic content. Add explicit === 0 guards in both resolveAppPageRscResponsePolicy and resolveAppPageHtmlResponsePolicy, placed before any branch that uses !revalidateSeconds or revalidateSeconds > 0. Also tighten the isForceStatic/isDynamicError condition in the HTML helper from (=== null || === 0) to === null, since 0 is now handled earlier.
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.
Clean, correct fix for a real caching bug. The JS falsiness of 0 silently skipped Cache-Control emission for revalidate = 0 pages — good catch.
The fix is well-placed in both functions, the tests cover the key cases including the isForceStatic + revalidateSeconds: 0 priority edge case, and the dead branch cleanup in the HTML helper is a nice follow-through.
Two minor suggestions (non-blocking):
-
RSC function consistency: The condition on line 94 (
!options.revalidateSeconds) and line 103 (if (options.revalidateSeconds)) still rely on truthiness checks. Now that0is handled above, they're safe —nullis the only remaining falsy value — but this is the same class of truthiness-relying code that caused the original bug. Consider tightening to=== nulland!== null && > 0for symmetry with the HTML helper, which you already cleaned up nicely. -
Test coverage: Consider adding an
isDynamicError: true+revalidateSeconds: 0test case alongside the existingisForceStatic: true+revalidateSeconds: 0cases, sinceisDynamicErroris the other flag in the condition the comments warn about.
| // force-dynamic for caching purposes. Must be checked before the | ||
| // isForceStatic/isDynamicError branch below, which uses !revalidateSeconds | ||
| // and would incorrectly catch 0 as a falsy value. | ||
| if (options.revalidateSeconds === 0) { |
There was a problem hiding this comment.
Fix looks correct. One small follow-up: the truthiness check on line 94 (!options.revalidateSeconds) is now safe since 0 exits above, but it's the same pattern that caused the original bug. For resilience against future reordering, consider tightening to === null:
| if (options.revalidateSeconds === 0) { | |
| if (options.revalidateSeconds === 0) { | |
| return { cacheControl: NO_STORE_CACHE_CONTROL }; | |
| } |
(The suggestion above is just the existing code — the real change would be on line 94 below, changing !options.revalidateSeconds to options.revalidateSeconds === null. Non-blocking.)
| }; | ||
| } | ||
|
|
||
| if ((options.isForceStatic || options.isDynamicError) && options.revalidateSeconds === null) { |
There was a problem hiding this comment.
Good cleanup — tightening from (=== null || === 0) to === null here is the right call now that 0 is handled above. This is exactly the kind of dead-branch removal that makes the precedence chain clearer.
| }), | ||
| ).toEqual({ | ||
| cacheControl: "no-store, must-revalidate", | ||
| }); |
There was a problem hiding this comment.
Nice coverage of the isForceStatic priority case. Consider also adding isDynamicError: true + revalidateSeconds: 0 since that's the other flag in the condition the comment warns about:
// revalidate = 0 takes priority over isDynamicError too
expect(
resolveAppPageRscResponsePolicy({
dynamicUsedDuringBuild: false,
isDynamicError: true,
isForceDynamic: false,
isForceStatic: false,
isProduction: true,
revalidateSeconds: 0,
}),
).toEqual({
cacheControl: "no-store, must-revalidate",
});
|
Review submitted successfully. Here's a summary of my review: Approved — this is a clean, correct fix for a real caching bug where JS falsiness of Two non-blocking suggestions:
|
2 participants
Summary
export const revalidate = 0were not getting aCache-Controlheader, allowing CDNs to cache them0is falsy in JS — the RSC function usedif (options.revalidateSeconds)and the HTML function usedrevalidateSeconds > 0, both silently skipping the zero case=== 0checks in bothresolveAppPageRscResponsePolicyandresolveAppPageHtmlResponsePolicy, placed before any branch that would mishandle0as falsyisForceStatic/isDynamicErrorcondition in the HTML helper from(=== null || === 0)to=== null, removing the now-unreachable dead branchrevalidate = 0as equivalent todynamic = "force-dynamic"for caching: emitsCache-Control: no-store, must-revalidateTest plan
revalidateSeconds: 0in both RSC and HTML policy functions, includingisForceStatic: true+revalidateSeconds: 0combination to verify priority orderingpnpm run checkpasses