Renew prefetch cache entry after update from server#61573
Merged
Conversation
This was referenced Feb 2, 2024
Member
Author
This stack of pull requests is managed by Graphite. Learn more about stacking. |
8563e34 to
a55d8b3
Compare
Member
Tests Passed |
a55d8b3 to
058c955
Compare
Member
Stats from current PRDefault BuildGeneral
Client Bundles (main, webpack)
Legacy Client Bundles (polyfills)
Client Pages
Client Build Manifests
Rendered Page Sizes
Edge SSR bundle Size
Middleware size
Next Runtimes
build cache
Diff detailsDiff for 1068-HASH.jsDiff too large to display |
86c1971 to
508f8a5
Compare
058c955 to
433aa22
Compare
70a9616 to
eae1430
Compare
433aa22 to
d3f1cc5
Compare
eae1430 to
bc62046
Compare
d3f1cc5 to
1645efa
Compare
95339b9 to
8544ddb
Compare
a61bdd8 to
7187ea6
Compare
17c9a20 to
ec7f968
Compare
7187ea6 to
4b2e379
Compare
ec7f968 to
407fa0e
Compare
4b2e379 to
4ad233e
Compare
Contributor
|
If this pull request is merged, I will close #58997. |
654953d to
1f62f69
Compare
4ad233e to
06c0610
Compare
acdlite
reviewed
Feb 13, 2024
packages/next/src/client/components/router-reducer/reducers/navigate-reducer.ts
Outdated
Show resolved
Hide resolved
1f62f69 to
8c37ac6
Compare
Base automatically changed from
02-01-seed_prefetch_cache_with_initial_page
to
canary
February 13, 2024 15:42
06c0610 to
ef85241
Compare
ef85241 to
b57039a
Compare
b57039a to
f507d83
Compare
acdlite
approved these changes
Feb 20, 2024
Contributor
acdlite
left a comment
There was a problem hiding this comment.
Looks great, thanks for the #DigDeep
| cacheNodeSeedData: CacheNodeSeedData | null, | ||
| head: React.ReactNode, | ||
| wasPrefetched?: boolean | ||
| hasReusablePrefetch?: boolean |
Contributor
There was a problem hiding this comment.
I think is argument is no longer needed? For the same reason you removed it from the PPR implementation
Member
Author
There was a problem hiding this comment.
I think it's still needed but for a more subtle reason -- we set this flag to true specifically for auto prefetches that don't have a reusable cache status. So that means fresh entries technically don't fall into this categorization.
When attempting to remove it I was noticing that once we renewed the stale prefetch, the lazy fetch was never happening, because fillLazyItemsTillLeafWithHead would always hit this condition since there would always be an existing cache node.
For now I'm going to leave it as-is but I'd like to better understand why fillLazyItemsTillLeafWithHead is triggering any lazy fetch behavior rather than fillCacheWithNewSubtreeData.
This file contains hidden or 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
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
4 participants
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.
What
When a prefetch cache entry becomes "stale", it'll remain stale until eventually it gets evicted. However during that stale window, the cache is never revalidated, and so instant navigations stop working and data is fetched from origin every navigation.
Why
The
lastUsedTimeentry on the prefetch cache is currently only updated after the first read. Once it becomes stale,applyFlightData's recursive functions will see that there is no longer a reusable prefetch cache entry, and will trigger a lazy fetch of the new data on every subsequent navigation.How
This updates prefetch cache handling to always ensure we’re using a fresh or reusable prefetch cache entry. This means that stale prefetch entries will be refreshed on a navigation event. As a result of this, I’ve had to disable one of our tests that relies on this stale cache behavior. It’s not ideal that we’re blocked on the loading boundary when fetching child segment data—ideally we can refactor this to cache the loading component in the CacheNode and copy it over on navigations, similar to how ‘head’ is handled. I’ll work on this in a separate PR.
Note: The new client cache test for this is disabled in PPR for the same reason as the other tests: auto prefetching with PPR navigations is currently loading fresh data rather than reusing the prefetch cache.
Fixes #58969
Fixes #58723
Closes NEXT-1904