fix: set netlify-cache-tag for not prerendered content #2495
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
📊 Package size report -0.02%↓
Unchanged files
🤖 This report was automatically generated by pkg-size-action |
pieh
force-pushed
the
fix/cache-tags-not-prerendered
branch
5 times, most recently
from
3bccb0f to
74983c0
Compare
pieh
added
the
test all versions
…ses for not prerendered content
…cases for not prerendered content
pieh
force-pushed
the
fix/cache-tags-not-prerendered
branch
from
74983c0 to
bd713be
Compare
… manifest created for prerendered pages
pieh
force-pushed
the
fix/cache-tags-not-prerendered
branch
from
6ce8636 to
4b6de32
Compare
pieh
changed the title
serhalp
previously approved these changes
Jun 21, 2024
| @@ -311,47 +311,6 @@ export const copyNextDependencies = async (ctx: PluginContext): Promise<void> => | |||
| }) | |||
| } | |||
|
|
|||
| export const writeTagsManifest = async (ctx: PluginContext): Promise<void> => { | |||
| const manifest = await ctx.getPrerenderManifest() | |||
There was a problem hiding this comment.
Looks like we can delete this too
There was a problem hiding this comment.
... Oh, I don't think this is related to your changes, but still dead code
There was a problem hiding this comment.
Yeah, not really related to this change, but I did remove it now
Comment on lines
+123
to
+131
| // Bail if we already have cache tags - `captureCacheTags()` is called on both `CacheHandler.get` and `CacheHandler.set` | ||
| // that's because `CacheHandler.get` might not have a cache value (cache miss or on-demand revalidation) in which case | ||
| // response is generated in blocking way and we need to capture cache tags from the cache value we are setting. | ||
| // If both `CacheHandler.get` and `CacheHandler.set` are called in the same request, we want to use cache tags from | ||
| // first `CacheHandler.get` and not from following `CacheHandler.set` as this is pattern for Stale-while-revalidate behavior | ||
| // and stale response is served while new one is generated. | ||
| if (requestContext.responseCacheTags) { | ||
| return | ||
| } |
There was a problem hiding this comment.
I'm trying to think of something less leaky but I can't 😞
Co-authored-by: Philippe Serhal <[email protected]>
pieh
force-pushed
the
fix/cache-tags-not-prerendered
branch
from
d1075a7 to
29607db
Compare
serhalp
approved these changes
Jun 24, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
We are currently relying on
tags-manifest.jsonto setnetlify-cache-tagresponse header. Tags Manifest is produced at build time from prerendered content so it won't contain any not prerendered pages which will result in this header missing for not prerendered pages and therefore make it impossible to purge CDN cache (it's still possible to invalidate Next.js cache, but if there were CDN nodes that already cached content - we aren't able to purge that right now). This might result in "random response" being served to user depending on which CDN handled the request (it might be fresh response or it might be old response that was cached)Tests
Added failing E2E test cases in first 2 commits for both App and Pages Router
Relevant links (GitHub issues, etc.) or a picture of cute animal
https://linear.app/netlify/issue/FRP-566/nextjs-routes-revalidating-when-not-required#comment-1bf42469