From be44437a34ec6e84506f43d129734c1f0fe46429 Mon Sep 17 00:00:00 2001 From: Zack Tanner Date: Wed, 13 Mar 2024 13:04:13 -0700 Subject: [PATCH] Fix instant loading states --- ...cache-node-data-for-segment-path.test.tsx} | 21 +--- ...clear-cache-node-data-for-segment-path.ts} | 17 ++- .../router-reducer/prefetch-cache-utils.ts | 7 +- .../reducers/navigate-reducer.ts | 108 +++++++++++++++--- .../app-client-cache/client-cache.test.ts | 4 +- test/ppr-tests-manifest.json | 1 - 6 files changed, 105 insertions(+), 53 deletions(-) rename packages/next/src/client/components/router-reducer/{fill-cache-with-data-property.test.tsx => clear-cache-node-data-for-segment-path.test.tsx} (84%) rename packages/next/src/client/components/router-reducer/{fill-cache-with-data-property.ts => clear-cache-node-data-for-segment-path.ts} (82%) diff --git a/packages/next/src/client/components/router-reducer/fill-cache-with-data-property.test.tsx b/packages/next/src/client/components/router-reducer/clear-cache-node-data-for-segment-path.test.tsx similarity index 84% rename from packages/next/src/client/components/router-reducer/fill-cache-with-data-property.test.tsx rename to packages/next/src/client/components/router-reducer/clear-cache-node-data-for-segment-path.test.tsx index 542be341f2eef..abb2df3d6beab 100644 --- a/packages/next/src/client/components/router-reducer/fill-cache-with-data-property.test.tsx +++ b/packages/next/src/client/components/router-reducer/clear-cache-node-data-for-segment-path.test.tsx @@ -1,18 +1,9 @@ import React from 'react' -import type { FetchServerResponseResult } from './fetch-server-response' -import { fillCacheWithDataProperty } from './fill-cache-with-data-property' +import { clearCacheNodeDataForSegmentPath } from './clear-cache-node-data-for-segment-path' import type { CacheNode } from '../../../shared/lib/app-router-context.shared-runtime' -describe('fillCacheWithDataProperty', () => { - it('should add data property', () => { - const fetchServerResponseMock: jest.Mock< - Promise - > = jest.fn(() => - Promise.resolve([ - /* TODO-APP: replace with actual FlightData */ '', - undefined, - ]) - ) +describe('clearCacheNodeDataForSegmentPath', () => { + it('should clear the data property', () => { const pathname = '/dashboard/settings' const segments = pathname.split('/') @@ -80,9 +71,7 @@ describe('fillCacheWithDataProperty', () => { ]), } - fillCacheWithDataProperty(cache, existingCache, flightSegmentPath, () => - fetchServerResponseMock() - ) + clearCacheNodeDataForSegmentPath(cache, existingCache, flightSegmentPath) expect(cache).toMatchInlineSnapshot(` { @@ -121,7 +110,7 @@ describe('fillCacheWithDataProperty', () => { }, "dashboard" => { "head": null, - "lazyData": Promise {}, + "lazyData": null, "lazyDataResolved": false, "loading": null, "parallelRoutes": Map {}, diff --git a/packages/next/src/client/components/router-reducer/fill-cache-with-data-property.ts b/packages/next/src/client/components/router-reducer/clear-cache-node-data-for-segment-path.ts similarity index 82% rename from packages/next/src/client/components/router-reducer/fill-cache-with-data-property.ts rename to packages/next/src/client/components/router-reducer/clear-cache-node-data-for-segment-path.ts index bc436b2fe6310..16ce5807e83d2 100644 --- a/packages/next/src/client/components/router-reducer/fill-cache-with-data-property.ts +++ b/packages/next/src/client/components/router-reducer/clear-cache-node-data-for-segment-path.ts @@ -1,16 +1,14 @@ -import type { FetchServerResponseResult } from './fetch-server-response' import type { FlightSegmentPath } from '../../../server/app-render/types' import type { CacheNode } from '../../../shared/lib/app-router-context.shared-runtime' import { createRouterCacheKey } from './create-router-cache-key' /** - * Kick off fetch based on the common layout between two routes. Fill cache with data property holding the in-progress fetch. + * This will clear the CacheNode data for a particular segment path. This will cause a lazy-fetch in layout router to fill in new data. */ -export function fillCacheWithDataProperty( +export function clearCacheNodeDataForSegmentPath( newCache: CacheNode, existingCache: CacheNode, - flightSegmentPath: FlightSegmentPath, - fetchResponse: () => Promise + flightSegmentPath: FlightSegmentPath ): void { const isLastEntry = flightSegmentPath.length <= 2 @@ -38,7 +36,7 @@ export function fillCacheWithDataProperty( childCacheNode === existingChildCacheNode ) { childSegmentMap.set(cacheKey, { - lazyData: fetchResponse(), + lazyData: null, rsc: null, prefetchRsc: null, head: null, @@ -55,7 +53,7 @@ export function fillCacheWithDataProperty( // Start fetch in the place where the existing cache doesn't have the data yet. if (!childCacheNode) { childSegmentMap.set(cacheKey, { - lazyData: fetchResponse(), + lazyData: null, rsc: null, prefetchRsc: null, head: null, @@ -82,10 +80,9 @@ export function fillCacheWithDataProperty( childSegmentMap.set(cacheKey, childCacheNode) } - return fillCacheWithDataProperty( + return clearCacheNodeDataForSegmentPath( childCacheNode, existingChildCacheNode, - flightSegmentPath.slice(2), - fetchResponse + flightSegmentPath.slice(2) ) } diff --git a/packages/next/src/client/components/router-reducer/prefetch-cache-utils.ts b/packages/next/src/client/components/router-reducer/prefetch-cache-utils.ts index c34b156fa657a..522ccc6aab11e 100644 --- a/packages/next/src/client/components/router-reducer/prefetch-cache-utils.ts +++ b/packages/next/src/client/components/router-reducer/prefetch-cache-utils.ts @@ -79,12 +79,7 @@ export function getOrCreatePrefetchCacheEntry({ existingCacheEntry.kind !== PrefetchKind.FULL && kind === PrefetchKind.FULL - // If the cache entry isn't reusable, rather than returning it, we want to create a new entry. - const hasReusablePrefetch = - existingCacheEntry.status === PrefetchCacheEntryStatus.reusable || - existingCacheEntry.status === PrefetchCacheEntryStatus.fresh - - if (switchedToFullPrefetch || !hasReusablePrefetch) { + if (switchedToFullPrefetch) { return createLazyPrefetchEntry({ tree, url, diff --git a/packages/next/src/client/components/router-reducer/reducers/navigate-reducer.ts b/packages/next/src/client/components/router-reducer/reducers/navigate-reducer.ts index 85439b5ca78c9..f364bd31e45d5 100644 --- a/packages/next/src/client/components/router-reducer/reducers/navigate-reducer.ts +++ b/packages/next/src/client/components/router-reducer/reducers/navigate-reducer.ts @@ -9,11 +9,12 @@ import { invalidateCacheBelowFlightSegmentPath } from '../invalidate-cache-below import { applyRouterStatePatchToTreeSkipDefault } from '../apply-router-state-patch-to-tree' import { shouldHardNavigate } from '../should-hard-navigate' import { isNavigatingToNewRootLayout } from '../is-navigating-to-new-root-layout' -import type { - Mutable, - NavigateAction, - ReadonlyReducerState, - ReducerState, +import { + PrefetchCacheEntryStatus, + type Mutable, + type NavigateAction, + type ReadonlyReducerState, + type ReducerState, } from '../router-reducer-types' import { handleMutable } from '../handle-mutable' import { applyFlightData } from '../apply-flight-data' @@ -28,6 +29,7 @@ import { getOrCreatePrefetchCacheEntry, prunePrefetchCache, } from '../prefetch-cache-utils' +import { clearCacheNodeDataForSegmentPath } from '../clear-cache-node-data-for-segment-path' export function handleExternalUrl( state: ReadonlyReducerState, @@ -69,6 +71,32 @@ function generateSegmentsFromPatch( return segments } +function triggerLazyFetchForLeafSegments( + newCache: CacheNode, + currentCache: CacheNode, + flightSegmentPath: FlightSegmentPath, + treePatch: FlightRouterState +) { + let appliedPatch = false + + newCache.rsc = currentCache.rsc + newCache.prefetchRsc = currentCache.prefetchRsc + newCache.loading = currentCache.loading + newCache.parallelRoutes = new Map(currentCache.parallelRoutes) + + const segmentPathsToFill = generateSegmentsFromPatch(treePatch).map( + (segment) => [...flightSegmentPath, ...segment] + ) + + for (const segmentPaths of segmentPathsToFill) { + clearCacheNodeDataForSegmentPath(newCache, currentCache, segmentPaths) + + appliedPatch = true + } + + return appliedPatch +} + // These implementations are expected to diverge significantly, so I've forked // the entire function. The one that's disabled should be dead code eliminated // because the check here is statically inlined at build time. @@ -109,10 +137,12 @@ function navigateReducer_noPPR( return data.then( ([flightData, canonicalUrlOverride]) => { + let isFirstRead = false // we only want to mark this once if (!prefetchValues.lastUsedTime) { // important: we should only mark the cache node as dirty after we unsuspend from the call above prefetchValues.lastUsedTime = Date.now() + isFirstRead = true } // Handle case when navigating to page in `pages` from `app` @@ -159,12 +189,33 @@ function navigateReducer_noPPR( } const cache: CacheNode = createEmptyCacheNode() - let applied = applyFlightData( - currentCache, - cache, - flightDataPath, - prefetchValues - ) + let applied = false + + if ( + prefetchValues.status === PrefetchCacheEntryStatus.stale && + !isFirstRead + ) { + // When we have a stale prefetch entry, we only want to re-use the loading state of the route we're navigating to, to support instant loading navigations + // this will trigger a lazy fetch for the actual page data by nulling the `rsc` and `prefetchRsc` values for page data, + // while copying over the `loading` for the segment that contains the page data. + // We only do this on subsequent reads, as otherwise there'd be no loading data to re-use. + applied = triggerLazyFetchForLeafSegments( + cache, + currentCache, + flightSegmentPath, + treePatch + ) + // since we re-used the stale cache's loading state & refreshed the data, + // update the `lastUsedTime` so that it can continue to be re-used for the next 30s + prefetchValues.lastUsedTime = Date.now() + } else { + applied = applyFlightData( + currentCache, + cache, + flightDataPath, + prefetchValues + ) + } const hardNavigate = shouldHardNavigate( // TODO-APP: remove '' @@ -252,10 +303,12 @@ function navigateReducer_PPR( return data.then( ([flightData, canonicalUrlOverride, _postponed]) => { + let isFirstRead = false // we only want to mark this once if (!prefetchValues.lastUsedTime) { // important: we should only mark the cache node as dirty after we unsuspend from the call above prefetchValues.lastUsedTime = Date.now() + isFirstRead = true } // Handle case when navigating to page in `pages` from `app` @@ -379,12 +432,33 @@ function navigateReducer_PPR( // tree. Or in the meantime we could factor it out into a // separate function. const cache: CacheNode = createEmptyCacheNode() - let applied = applyFlightData( - currentCache, - cache, - flightDataPath, - prefetchValues - ) + let applied = false + + if ( + prefetchValues.status === PrefetchCacheEntryStatus.stale && + !isFirstRead + ) { + // When we have a stale prefetch entry, we only want to re-use the loading state of the route we're navigating to, to support instant loading navigations + // this will trigger a lazy fetch for the actual page data by nulling the `rsc` and `prefetchRsc` values for page data, + // while copying over the `loading` for the segment that contains the page data. + // We only do this on subsequent reads, as otherwise there'd be no loading data to re-use. + applied = triggerLazyFetchForLeafSegments( + cache, + currentCache, + flightSegmentPath, + treePatch + ) + // since we re-used the stale cache's loading state & refreshed the data, + // update the `lastUsedTime` so that it can continue to be re-used for the next 30s + prefetchValues.lastUsedTime = Date.now() + } else { + applied = applyFlightData( + currentCache, + cache, + flightDataPath, + prefetchValues + ) + } const hardNavigate = shouldHardNavigate( // TODO-APP: remove '' diff --git a/test/e2e/app-dir/app-client-cache/client-cache.test.ts b/test/e2e/app-dir/app-client-cache/client-cache.test.ts index 773da732f14eb..99cabd73cb027 100644 --- a/test/e2e/app-dir/app-client-cache/client-cache.test.ts +++ b/test/e2e/app-dir/app-client-cache/client-cache.test.ts @@ -373,9 +373,7 @@ createNextDescribe( expect(newNumber).toBe(initialNumber) }) - // TODO: Rather than reusing parts of a stale prefetch cache entry to make this work, - // we should be able to copy over the existing loading from a previous cache node on navigation. - it.skip('should refetch below the fold after 30 seconds', async () => { + it('should refetch below the fold after 30 seconds', async () => { const randomLoadingNumber = await browser .elementByCss('[href="/1?timeout=1000"]') .click() diff --git a/test/ppr-tests-manifest.json b/test/ppr-tests-manifest.json index 170ead9c5e1ef..dd2e05e89fa70 100644 --- a/test/ppr-tests-manifest.json +++ b/test/ppr-tests-manifest.json @@ -12,7 +12,6 @@ "test/e2e/app-dir/app-client-cache/client-cache.test.ts": { "failed": [ "app dir client cache semantics prefetch={undefined} - default should re-use the full cache for only 30 seconds", - "app dir client cache semantics prefetch={undefined} - default should refetch below the fold after 30 seconds", "app dir client cache semantics prefetch={undefined} - default should renew the 30s cache once the data is revalidated", "app dir client cache semantics prefetch={undefined} - default should refetch the full page after 5 mins" ]