From 77d72f342d96746fc0f34c2d307061ccdc7c70a3 Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Wed, 11 Dec 2024 21:59:51 -0500 Subject: [PATCH] Rename variables in LayoutRouter for clarity LayoutRouter has changed significantly since it was originally written and the structure has become harder to follow. One thing I always find confusing whenever I'm reading this code is that some of the values correspond the the *parent* segment, while others correspond to the *current* segment. So this moves the code around a bit and updates the names so it's clearer which parts belong to which segments. While working on this, I noticed a funny thing about how loading boundaries work that was made more obvious by the revised naming. I've left a TODO comment to follow up on whether this was intentional. --- .../next/src/client/components/app-router.tsx | 7 +- .../src/client/components/layout-router.tsx | 109 +++++++++--------- .../next/src/client/components/navigation.ts | 2 +- .../lib/app-router-context.shared-runtime.ts | 5 +- .../acceptance-app/hydration-error.test.ts | 78 ++++++------- .../dynamic-io-dev-errors.test.ts | 2 +- 6 files changed, 103 insertions(+), 100 deletions(-) diff --git a/packages/next/src/client/components/app-router.tsx b/packages/next/src/client/components/app-router.tsx index 93c023294995c9..23cb1803103a3f 100644 --- a/packages/next/src/client/components/app-router.tsx +++ b/packages/next/src/client/components/app-router.tsx @@ -564,14 +564,13 @@ function Router({ const layoutRouterContext = useMemo(() => { return { - childNodes: cache.parallelRoutes, - tree, + parentTree: tree, + parentCacheNode: cache, // Root node always has `url` // Provided in AppTreeContext to ensure it can be overwritten in layout-router url: canonicalUrl, - loading: cache.loading, } - }, [cache.parallelRoutes, tree, canonicalUrl, cache.loading]) + }, [tree, cache, canonicalUrl]) const globalLayoutRouterContext = useMemo(() => { return { diff --git a/packages/next/src/client/components/layout-router.tsx b/packages/next/src/client/components/layout-router.tsx index d2ecca2be5ccf2..5319797627790d 100644 --- a/packages/next/src/client/components/layout-router.tsx +++ b/packages/next/src/client/components/layout-router.tsx @@ -1,7 +1,7 @@ 'use client' import type { - ChildSegmentMap, + CacheNode, LazyCacheNode, LoadingModuleData, } from '../../shared/lib/app-router-context.shared-runtime' @@ -314,19 +314,15 @@ function ScrollAndFocusHandler({ * InnerLayoutRouter handles rendering the provided segment based on the cache. */ function InnerLayoutRouter({ - parallelRouterKey, - url, - childNodes, - segmentPath, tree, - cacheKey, + segmentPath, + cacheNode, + url, }: { - parallelRouterKey: string - url: string - childNodes: ChildSegmentMap - segmentPath: FlightSegmentPath tree: FlightRouterState - cacheKey: ReturnType + segmentPath: FlightSegmentPath + cacheNode: CacheNode + url: string }) { const context = useContext(GlobalLayoutRouterContext) if (!context) { @@ -335,29 +331,6 @@ function InnerLayoutRouter({ const { changeByServerResponse, tree: fullTree } = context - // Read segment path from the parallel router cache node. - let childNode = childNodes.get(cacheKey) - - // When data is not available during rendering client-side we need to fetch - // it from the server. - if (childNode === undefined) { - const newLazyCacheNode: LazyCacheNode = { - lazyData: null, - rsc: null, - prefetchRsc: null, - head: null, - prefetchHead: null, - parallelRoutes: new Map(), - loading: null, - } - - /** - * Flight data fetch kicked off during render and put into the cache. - */ - childNode = newLazyCacheNode - childNodes.set(cacheKey, newLazyCacheNode) - } - // `rsc` represents the renderable node for this segment. // If this segment has a `prefetchRsc`, it's the statically prefetched data. @@ -366,7 +339,7 @@ function InnerLayoutRouter({ // // If no prefetch data is available, then we go straight to rendering `rsc`. const resolvedPrefetchRsc = - childNode.prefetchRsc !== null ? childNode.prefetchRsc : childNode.rsc + cacheNode.prefetchRsc !== null ? cacheNode.prefetchRsc : cacheNode.rsc // We use `useDeferredValue` to handle switching between the prefetched and // final values. The second argument is returned on initial render, then it @@ -375,7 +348,7 @@ function InnerLayoutRouter({ // @ts-expect-error The second argument to `useDeferredValue` is only // available in the experimental builds. When its disabled, it will always // return `rsc`. - const rsc: any = useDeferredValue(childNode.rsc, resolvedPrefetchRsc) + const rsc: any = useDeferredValue(cacheNode.rsc, resolvedPrefetchRsc) // `rsc` is either a React node or a promise for a React node, except we // special case `null` to represent that this segment's data is missing. If @@ -392,7 +365,7 @@ function InnerLayoutRouter({ // the server and patch the cache. // Check if there's already a pending request. - let lazyData = childNode.lazyData + let lazyData = cacheNode.lazyData if (lazyData === null) { /** * Router state with refetch marker added @@ -400,7 +373,7 @@ function InnerLayoutRouter({ // TODO-APP: remove '' const refetchTree = walkAddRefetch(['', ...segmentPath], fullTree) const includeNextUrl = hasInterceptionRouteInCurrentTree(fullTree) - childNode.lazyData = lazyData = fetchServerResponse( + cacheNode.lazyData = lazyData = fetchServerResponse( new URL(url, location.origin), { flightRouterState: refetchTree, @@ -427,11 +400,11 @@ function InnerLayoutRouter({ // The layout router context narrows down tree and childNodes at each level. {resolvedRsc} @@ -528,20 +501,23 @@ export default function OuterLayoutRouter({ throw new Error('invariant expected layout router to be mounted') } - const { childNodes, tree, url, loading } = context + const { parentTree, parentCacheNode, url } = context - // Get the current parallelRouter cache node - let childNodesForParallelRouter = childNodes.get(parallelRouterKey) + // Get the CacheNode for this segment by reading it from the parent segment's + // child map. + const parentParallelRoutes = parentCacheNode.parallelRoutes + let segmentMap = parentParallelRoutes.get(parallelRouterKey) // If the parallel router cache node does not exist yet, create it. // This writes to the cache when there is no item in the cache yet. It never *overwrites* existing cache items which is why it's safe in concurrent mode. - if (!childNodesForParallelRouter) { - childNodesForParallelRouter = new Map() - childNodes.set(parallelRouterKey, childNodesForParallelRouter) + if (!segmentMap) { + segmentMap = new Map() + parentParallelRoutes.set(parallelRouterKey, segmentMap) } // Get the active segment in the tree // The reason arrays are used in the data format is that these are transferred from the server to the browser so it's optimized to save bytes. - const treeSegment = tree[1][parallelRouterKey][0] + const tree = parentTree[1][parallelRouterKey] + const treeSegment = tree[0] // The "state" key of a segment is the one passed to React — it represents the // identity of the UI tree. Whenever the state key changes, the tree is @@ -556,6 +532,26 @@ export default function OuterLayoutRouter({ const cacheKey = createRouterCacheKey(treeSegment) const stateKey = createRouterCacheKey(treeSegment, true) // no search params + // Read segment path from the parallel router cache node. + let cacheNode = segmentMap.get(cacheKey) + if (cacheNode === undefined) { + // When data is not available during rendering client-side we need to fetch + // it from the server. + const newLazyCacheNode: LazyCacheNode = { + lazyData: null, + rsc: null, + prefetchRsc: null, + head: null, + prefetchHead: null, + parallelRoutes: new Map(), + loading: null, + } + + // Flight data fetch kicked off during render and put into the cache. + cacheNode = newLazyCacheNode + segmentMap.set(cacheKey, newLazyCacheNode) + } + /* - Error boundary - Only renders error boundary if error component is provided. @@ -565,6 +561,17 @@ export default function OuterLayoutRouter({ - Rendered for each segment to ensure they have their own loading state. - Passed to the router during rendering to ensure it can be immediately rendered when suspending on a Flight fetch. */ + + // TODO: The loading module data for a segment is stored on the parent, then + // applied to each of that parent segment's parallel route slots. In the + // simple case where there's only one parallel route (the `children` slot), + // this is no different from if the loading module data where stored on the + // child directly. But I'm not sure this actually makes sense when there are + // multiple parallel routes. It's not a huge issue because you always have + // the option to define a narrower loading boundary for a particular slot. But + // this sort of smells like an implementation accident to me. + const loadingModuleData = parentCacheNode.loading + return ( - + diff --git a/packages/next/src/client/components/navigation.ts b/packages/next/src/client/components/navigation.ts index 94d4d2414e8d1c..33e318994a1135 100644 --- a/packages/next/src/client/components/navigation.ts +++ b/packages/next/src/client/components/navigation.ts @@ -221,7 +221,7 @@ export function useSelectedLayoutSegments( // @ts-expect-error This only happens in `pages`. Type is overwritten in navigation.d.ts if (!context) return null - return getSelectedLayoutSegmentPath(context.tree, parallelRouteKey) + return getSelectedLayoutSegmentPath(context.parentTree, parallelRouteKey) } /** diff --git a/packages/next/src/shared/lib/app-router-context.shared-runtime.ts b/packages/next/src/shared/lib/app-router-context.shared-runtime.ts index 2ee50c11e45d08..7b1bcb941fdf24 100644 --- a/packages/next/src/shared/lib/app-router-context.shared-runtime.ts +++ b/packages/next/src/shared/lib/app-router-context.shared-runtime.ts @@ -146,10 +146,9 @@ export const AppRouterContext = React.createContext( null ) export const LayoutRouterContext = React.createContext<{ - childNodes: CacheNode['parallelRoutes'] - tree: FlightRouterState + parentTree: FlightRouterState + parentCacheNode: CacheNode url: string - loading: LoadingModuleData | Promise } | null>(null) export const GlobalLayoutRouterContext = React.createContext<{ diff --git a/test/development/acceptance-app/hydration-error.test.ts b/test/development/acceptance-app/hydration-error.test.ts index b9052eabc5f9a8..e3c290ea9e1260 100644 --- a/test/development/acceptance-app/hydration-error.test.ts +++ b/test/development/acceptance-app/hydration-error.test.ts @@ -425,28 +425,28 @@ describe('Error overlay for hydration errors in App router', () => { expect(await getRedboxTotalErrorCount(browser)).toBe(1) expect(await session.getRedboxDescription()).toMatchInlineSnapshot(` - "In HTML, whitespace text nodes cannot be a child of . Make sure you don't have any extra whitespace between tags on each line of your source code. - This will cause a hydration error. - - ... - - - - - - - - - - - - - >
- > {" "} - ... +"In HTML, whitespace text nodes cannot be a child of
. Make sure you don't have any extra whitespace between tags on each line of your source code. +This will cause a hydration error. + + ... + + + + + + + + + + + + +>
+> {" "} ... - " - `) + ... +" +`) // FIXME: fix the `pseudoHtml` should be extracted from the description // const pseudoHtml = await session.getRedboxComponentStack() @@ -907,24 +907,24 @@ describe('Error overlay for hydration errors in App router', () => { `) } else { expect(fullPseudoHtml).toMatchInlineSnapshot(` - "... - - - - - - -
-
-
-
- -

- - ... - + client - - server" - `) +"... + + + + + + +

+
+
+
+ +

+ + ... ++ client +- server" +`) } }) }) diff --git a/test/development/app-dir/dynamic-io-dev-errors/dynamic-io-dev-errors.test.ts b/test/development/app-dir/dynamic-io-dev-errors/dynamic-io-dev-errors.test.ts index 952ccda61ca7bc..fa374b7eb7ee31 100644 --- a/test/development/app-dir/dynamic-io-dev-errors/dynamic-io-dev-errors.test.ts +++ b/test/development/app-dir/dynamic-io-dev-errors/dynamic-io-dev-errors.test.ts @@ -77,7 +77,7 @@ describe('Dynamic IO Dev Errors', () => { '\n at html ()' + '\n at Root [Server] ()' : // TODO(veil): Should be ignore-listed (see https://linear.app/vercel/issue/NDX-464/next-internals-not-ignore-listed-in-terminal-in-webpack#comment-1164a36a) - '\n at parallelRouterKey (..') + '\n at tree (..') ) const description = await getRedboxDescription(browser)