Skip to content

Commit

Permalink
Fix PPR navigations & revert layerAssets property from FlightData (#…
Browse files Browse the repository at this point in the history
…67435)

This PR was originally intended to ensure style/script tags were always
rendered for all visible parallel routes, as before only the "first"
child was rendered, similar to `head`.

However, it caused a regression in PPR navigations, as the pending task
on the `layerAssets` CacheNode meant that the `useDeferredValue` call in
`layout-router` wouldn't be able to immediately switch to the prefetched
RSC payload, as it was blocked by the dynamic response.

Since `rsc` and `layerAssets` are both created by the same thing, this
stack of PRs rewinds some of that work to combine them.

This PR only reverts the previous implementation, and adds a test for
the PPR navigation case that was failing.

**Note: While I've split these PRs out for readability, this should land
with #67436 as this PR by itself does not restore the CSS behavior that
was fixed in the original implementation.**
  • Loading branch information
ztanner authored Jul 9, 2024
1 parent a123983 commit 19316b7
Show file tree
Hide file tree
Showing 32 changed files with 266 additions and 386 deletions.
24 changes: 0 additions & 24 deletions packages/next/src/client/components/app-router.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,6 @@ type AppRouterProps = Omit<
> & {
buildId: string
initialHead: ReactNode
initialLayerAssets: ReactNode
assetPrefix: string
missingSlots: Set<string>
}
Expand Down Expand Up @@ -153,8 +152,6 @@ export function createEmptyCacheNode(): CacheNode {
rsc: null,
prefetchRsc: null,
head: null,
layerAssets: null,
prefetchLayerAssets: null,
prefetchHead: null,
parallelRoutes: new Map(),
loading: null,
Expand Down Expand Up @@ -261,7 +258,6 @@ function Head({
function Router({
buildId,
initialHead,
initialLayerAssets,
initialTree,
initialCanonicalUrl,
initialSeedData,
Expand All @@ -279,7 +275,6 @@ function Router({
initialParallelRoutes,
location: !isServer ? window.location : null,
initialHead,
initialLayerAssets,
couldBeIntercepted,
}),
[
Expand All @@ -288,7 +283,6 @@ function Router({
initialCanonicalUrl,
initialTree,
initialHead,
initialLayerAssets,
couldBeIntercepted,
]
)
Expand Down Expand Up @@ -637,27 +631,9 @@ function Router({
head = null
}

// We use `useDeferredValue` to handle switching between the prefetched and
// final values. The second argument is returned on initial render, then it
// re-renders with the first argument. We only use the prefetched layer assets
// if they are available. Otherwise, we use the non-prefetched version.
const resolvedPrefetchLayerAssets =
cache.prefetchLayerAssets !== null
? cache.prefetchLayerAssets
: cache.layerAssets

const layerAssets = useDeferredValue(
cache.layerAssets,
// @ts-expect-error The second argument to `useDeferredValue` is only
// available in the experimental builds. When its disabled, it will always
// return `cache.layerAssets`.
resolvedPrefetchLayerAssets
)

let content = (
<RedirectBoundary>
{head}
{layerAssets}
{cache.rsc}
<AppRouterAnnouncer tree={tree} />
</RedirectBoundary>
Expand Down
23 changes: 0 additions & 23 deletions packages/next/src/client/components/layout-router.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -350,8 +350,6 @@ function InnerLayoutRouter({
rsc: null,
prefetchRsc: null,
head: null,
layerAssets: null,
prefetchLayerAssets: null,
prefetchHead: null,
parallelRoutes: new Map(),
loading: null,
Expand Down Expand Up @@ -427,23 +425,6 @@ function InnerLayoutRouter({
use(unresolvedThenable) as never
}

// We use `useDeferredValue` to handle switching between the prefetched and
// final values. The second argument is returned on initial render, then it
// re-renders with the first argument. We only use the prefetched layer assets
// if they are available. Otherwise, we use the non-prefetched version.
const resolvedPrefetchLayerAssets =
childNode.prefetchLayerAssets !== null
? childNode.prefetchLayerAssets
: childNode.layerAssets

const layerAssets = useDeferredValue(
childNode.layerAssets,
// @ts-expect-error The second argument to `useDeferredValue` is only
// available in the experimental builds. When its disabled, it will always
// return `cache.layerAssets`.
resolvedPrefetchLayerAssets
)

// If we get to this point, then we know we have something we can render.
const subtree = (
// The layout router context narrows down tree and childNodes at each level.
Expand All @@ -456,7 +437,6 @@ function InnerLayoutRouter({
loading: childNode.loading,
}}
>
{layerAssets}
{resolvedRsc}
</LayoutRouterContext.Provider>
)
Expand Down Expand Up @@ -517,7 +497,6 @@ export default function OuterLayoutRouter({
template,
notFound,
notFoundStyles,
styles,
}: {
parallelRouterKey: string
segmentPath: FlightSegmentPath
Expand All @@ -529,7 +508,6 @@ export default function OuterLayoutRouter({
template: React.ReactNode
notFound: React.ReactNode | undefined
notFoundStyles: React.ReactNode | undefined
styles?: React.ReactNode
}) {
const context = useContext(LayoutRouterContext)
if (!context) {
Expand Down Expand Up @@ -562,7 +540,6 @@ export default function OuterLayoutRouter({

return (
<>
{styles}
{preservedSegments.map((preservedSegment) => {
const preservedSegmentValue = getSegmentValue(preservedSegment)
const cacheKey = createRouterCacheKey(preservedSegment)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,15 +11,14 @@ export function applyFlightData(
prefetchEntry?: PrefetchCacheEntry
): boolean {
// The one before last item is the router state tree patch
const [treePatch, cacheNodeSeedData, head, layerAssets] =
flightDataPath.slice(-4)
const [treePatch, cacheNodeSeedData, head] = flightDataPath.slice(-3)

// Handles case where prefetch only returns the router tree patch without rendered components.
if (cacheNodeSeedData === null) {
return false
}

if (flightDataPath.length === 4) {
if (flightDataPath.length === 3) {
const rsc = cacheNodeSeedData[2]
const loading = cacheNodeSeedData[3]
cache.loading = loading
Expand All @@ -36,7 +35,6 @@ export function applyFlightData(
treePatch,
cacheNodeSeedData,
head,
layerAssets,
prefetchEntry
)
} else {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,6 @@ describe('clearCacheNodeDataForSegmentPath', () => {
rsc: null,
prefetchRsc: null,
head: null,
layerAssets: null,
prefetchLayerAssets: null,
prefetchHead: null,
parallelRoutes: new Map(),
loading: null,
Expand All @@ -28,8 +26,6 @@ describe('clearCacheNodeDataForSegmentPath', () => {
rsc: <>Root layout</>,
prefetchRsc: null,
head: null,
layerAssets: null,
prefetchLayerAssets: null,
prefetchHead: null,
loading: null,
parallelRoutes: new Map([
Expand All @@ -43,8 +39,6 @@ describe('clearCacheNodeDataForSegmentPath', () => {
rsc: <>Linking</>,
prefetchRsc: null,
head: null,
layerAssets: null,
prefetchLayerAssets: null,
prefetchHead: null,
loading: null,
parallelRoutes: new Map([
Expand All @@ -58,8 +52,6 @@ describe('clearCacheNodeDataForSegmentPath', () => {
rsc: <>Page</>,
prefetchRsc: null,
head: null,
layerAssets: null,
prefetchLayerAssets: null,
prefetchHead: null,
parallelRoutes: new Map(),
loading: null,
Expand All @@ -80,26 +72,22 @@ describe('clearCacheNodeDataForSegmentPath', () => {
expect(cache).toMatchInlineSnapshot(`
{
"head": null,
"layerAssets": null,
"lazyData": null,
"loading": null,
"parallelRoutes": Map {
"children" => Map {
"linking" => {
"head": null,
"layerAssets": null,
"lazyData": null,
"loading": null,
"parallelRoutes": Map {
"children" => Map {
"" => {
"head": null,
"layerAssets": null,
"lazyData": null,
"loading": null,
"parallelRoutes": Map {},
"prefetchHead": null,
"prefetchLayerAssets": null,
"prefetchRsc": null,
"rsc": <React.Fragment>
Page
Expand All @@ -108,27 +96,23 @@ describe('clearCacheNodeDataForSegmentPath', () => {
},
},
"prefetchHead": null,
"prefetchLayerAssets": null,
"prefetchRsc": null,
"rsc": <React.Fragment>
Linking
</React.Fragment>,
},
"dashboard" => {
"head": null,
"layerAssets": null,
"lazyData": null,
"loading": null,
"parallelRoutes": Map {},
"prefetchHead": null,
"prefetchLayerAssets": null,
"prefetchRsc": null,
"rsc": null,
},
},
},
"prefetchHead": null,
"prefetchLayerAssets": null,
"prefetchRsc": null,
"rsc": null,
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -40,8 +40,6 @@ export function clearCacheNodeDataForSegmentPath(
rsc: null,
prefetchRsc: null,
head: null,
layerAssets: null,
prefetchLayerAssets: null,
prefetchHead: null,
parallelRoutes: new Map(),
loading: null,
Expand All @@ -58,8 +56,6 @@ export function clearCacheNodeDataForSegmentPath(
rsc: null,
prefetchRsc: null,
head: null,
layerAssets: null,
prefetchLayerAssets: null,
prefetchHead: null,
parallelRoutes: new Map(),
loading: null,
Expand All @@ -74,8 +70,6 @@ export function clearCacheNodeDataForSegmentPath(
rsc: childCacheNode.rsc,
prefetchRsc: childCacheNode.prefetchRsc,
head: childCacheNode.head,
layerAssets: childCacheNode.layerAssets,
prefetchLayerAssets: childCacheNode.prefetchLayerAssets,
prefetchHead: childCacheNode.prefetchHead,
parallelRoutes: new Map(childCacheNode.parallelRoutes),
loading: childCacheNode.loading,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,6 @@ describe('createInitialRouterState', () => {
initialParallelRoutes,
location: new URL('/linking', 'https://localhost') as any,
initialHead: <title>Test</title>,
initialLayerAssets: null,
couldBeIntercepted: false,
})

Expand All @@ -53,16 +52,13 @@ describe('createInitialRouterState', () => {
initialParallelRoutes,
location: new URL('/linking', 'https://localhost') as any,
initialHead: <title>Test</title>,
initialLayerAssets: null,
})

const expectedCache: CacheNode = {
lazyData: null,
rsc: children,
prefetchRsc: null,
head: null,
layerAssets: null,
prefetchLayerAssets: null,
prefetchHead: null,
loading: null,
parallelRoutes: new Map([
Expand All @@ -85,8 +81,6 @@ describe('createInitialRouterState', () => {
parallelRoutes: new Map(),
loading: null,
head: <title>Test</title>,
layerAssets: null,
prefetchLayerAssets: null,
prefetchHead: null,
},
],
Expand All @@ -97,8 +91,6 @@ describe('createInitialRouterState', () => {
rsc: null,
prefetchRsc: null,
head: null,
layerAssets: null,
prefetchLayerAssets: null,
prefetchHead: null,
loading: null,
},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@ export interface InitialRouterStateParameters {
initialParallelRoutes: CacheNode['parallelRoutes']
location: Location | null
initialHead: ReactNode
initialLayerAssets: ReactNode
couldBeIntercepted?: boolean
}

Expand All @@ -33,7 +32,6 @@ export function createInitialRouterState({
initialParallelRoutes,
location,
initialHead,
initialLayerAssets,
couldBeIntercepted,
}: InitialRouterStateParameters) {
const isServer = !location
Expand All @@ -44,8 +42,6 @@ export function createInitialRouterState({
rsc: rsc,
prefetchRsc: null,
head: null,
layerAssets: initialLayerAssets,
prefetchLayerAssets: null,
prefetchHead: null,
// The cache gets seeded during the first render. `initialParallelRoutes` ensures the cache from the first render is there during the second render.
parallelRoutes: isServer ? new Map() : initialParallelRoutes,
Expand All @@ -71,8 +67,7 @@ export function createInitialRouterState({
undefined,
initialTree,
initialSeedData,
initialHead,
initialLayerAssets
initialHead
)
}

Expand Down Expand Up @@ -110,9 +105,7 @@ export function createInitialRouterState({
location.origin
)

const initialFlightData: FlightData = [
['', initialTree, null, null, initialLayerAssets],
]
const initialFlightData: FlightData = [['', initialTree, null, null]]
createPrefetchCacheEntryForInitialLoad({
url,
kind: PrefetchKind.AUTO,
Expand Down
Loading

0 comments on commit 19316b7

Please sign in to comment.