fix: create cache entries for fallback pages to support next@canary #2649
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.03%↑
Unchanged files
🤖 This report was automatically generated by pkg-size-action |
pieh
added
the
test all versions
pieh
force-pushed
the
michalpiechowiak/frb-1360-v15-canary-dynamic-routes-with-fallback-true-are-failing
branch
14 times, most recently
from
44abd9c to
9fc5e34
Compare
pieh
commented
Oct 10, 2024
pieh
commented
Oct 10, 2024
src/run/next.cts
Outdated
| const store = getRegionalBlobStore() | ||
| const relPath = relative(resolve('.next/server/pages'), path) | ||
| const relPath = relative(resolve(join(fsBlobsManifest.outputRoot, '/server/pages')), path) |
There was a problem hiding this comment.
I think this was not actually working correctly if user had non-default distDir (aka not .next) and also likely not working in monorepos
There was a problem hiding this comment.
this makes sense, however the outputRoot seems to have nothing to do with blob manifest so I'm wondering if there should be a different way to specify this value?
There was a problem hiding this comment.
given #2649 (comment) - I'll drop this change and keep hardcoded .next here as out of scope for this PR - I still think this is an issue, but I will not try to fix it in same PR that introduces creation of blob cache entries for fallbacks
pieh
force-pushed
the
michalpiechowiak/frb-1360-v15-canary-dynamic-routes-with-fallback-true-are-failing
branch
from
e6674ee to
df703e8
Compare
pieh
commented
Oct 11, 2024
| if (requestContext.responseCacheTags) { | ||
| if ( | ||
| requestContext.responseCacheTags && | ||
| (headers.has('cache-control') || headers.has('netlify-cdn-cache-control')) |
There was a problem hiding this comment.
this is to prevent setting tags on uncacheable responses when fallback in next@canary is served
orinokai
reviewed
Oct 15, 2024
There was a problem hiding this comment.
@serhalp and I have just been through the PR and generally it seems to do what it needs to, but we were wondering about the added complexity of having the fallback paths read from to the blob manifest file, rather than just having this check the blob store?
src/build/content/prerendered.ts
Outdated
| // Netlify Forms are not support and require a workaround | ||
| if (value.kind === 'PAGE' || value.kind === 'PAGES' || value.kind === 'APP_PAGE') { | ||
| verifyNetlifyForms(ctx, value.html) | ||
| } |
There was a problem hiding this comment.
not a blocker, but i'd argue that the Netlify Forms verification here is not adding much in terms of user value and we can strip it out to reduce noise (also APP_PAGE can never be true)
There was a problem hiding this comment.
Yeah, I'll drop that completely form here - we actually already are doing this right now because of https://github.com/netlify/next-runtime/blob/5f5ec0651726abf1027319707beaa1580a0c5dd8/src/build/content/static.ts#L35 so this was just doing same check again on same content
This was just copied from above code heh
src/run/next.cts
Outdated
| const store = getRegionalBlobStore() | ||
| const relPath = relative(resolve('.next/server/pages'), path) | ||
| const relPath = relative(resolve(join(fsBlobsManifest.outputRoot, '/server/pages')), path) |
There was a problem hiding this comment.
this makes sense, however the outputRoot seems to have nothing to do with blob manifest so I'm wondering if there should be a different way to specify this value?
Co-authored-by: Rob Stanford <[email protected]>
How about I change things so the type HtmlBlob {
html: string
isFallback: boolean
}Then we get away of the need to create the manifest that is currently created for the most part (except for The main reason (which is not great one heh) that I produced blobs fallback manifest, because we were handling |
…1360-v15-canary-dynamic-routes-with-fallback-true-are-failing
Great, this sounds like a good solution. Thanks @pieh. |
pieh
force-pushed
the
michalpiechowiak/frb-1360-v15-canary-dynamic-routes-with-fallback-true-are-failing
branch
from
3d0bc88 to
4198945
Compare
…oolean wether that is fallback html in single blob
pieh
force-pushed
the
michalpiechowiak/frb-1360-v15-canary-dynamic-routes-with-fallback-true-are-failing
branch
from
4198945 to
e5de50e
Compare
…1360-v15-canary-dynamic-routes-with-fallback-true-are-failing
orinokai
approved these changes
Oct 16, 2024
| @@ -11,7 +11,7 @@ export type RequestContext = { | |||
| responseCacheGetLastModified?: number | |||
| responseCacheKey?: string | |||
| responseCacheTags?: string[] | |||
| usedFsRead?: boolean | |||
| usedFsReadForNonFallback?: boolean | |||
There was a problem hiding this comment.
not blocking, but consider calling this something like usedFsReadForStaticRoute
There was a problem hiding this comment.
I think I'll merge this as-is to get this over with.
Minor side comment is that it hopefully now is actually functionally equivalent ( usedFsReadForNonFallback <=> usedFsReadForStaticRoute) but there's a chance there is another case here that we will be trying to handle in the future :D In this case I can at least say that usedFsReadForNonFallback name is actually correct because it literally check if html is fallback or not, but I am actually not 100% sure that all non-fallbacks are actually "StaticRoutes" heh
…-routes-with-fallback-true-are-failing
pieh
deleted the
michalpiechowiak/frb-1360-v15-canary-dynamic-routes-with-fallback-true-are-failing
branch
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
https://github.com/vercel/next.js/pull/68603/files#diff-6f4291cc2bfc5073fdca12a014011769e840ee68583db1468acef075f037015aL2826 this change moved from serving fallback directly from fs to using cache-handler
We were not creating cache entries for fallbacks which was resulting in
Error: invariant: cache entry required but not generatederrors as Next now expected those fallbacks to exist.This adds handling of fallback pages at build time and create cache entries for them which fixes above error.
Additionally - before that change in Next.js when fs was used to serve fallback - we were wrongly applying permament caching header for it (because of handling we have for fully static pages that is also using fs reads). This adds more nuance to that logic to not apply permament caching headers for fallback html reads
Documentation
Tests
Test cases added to e2e
Relevant links (GitHub issues, etc.) or a picture of cute animal
https://linear.app/netlify/issue/FRB-1360/[v15-canary]-dynamic-routes-with-fallback-true-are-failing-with-cache
fixes #2627