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.
vercel · Dec 12, 2024 · 77d72f3 · 77d72f3
1 parent effaba7
commit 77d72f3
Show file tree

Hide file tree

Showing 6 changed files with 103 additions and 100 deletions.
diff --git 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
@@ -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<typeof createRouterCacheKey>
+  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,15 +365,15 @@ 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
        */
       // 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.
     <LayoutRouterContext.Provider
       value={{
-        tree: tree[1][parallelRouterKey],
-        childNodes: childNode.parallelRoutes,
+        parentTree: tree,
+        parentCacheNode: cacheNode,
+
         // TODO-APP: overriding of url for parallel routes
         url: url,
-        loading: childNode.loading,
       }}
     >
       {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 (
     <TemplateContext.Provider
       key={stateKey}
@@ -575,20 +582,18 @@ export default function OuterLayoutRouter({
             errorStyles={errorStyles}
             errorScripts={errorScripts}
           >
-            <LoadingBoundary loading={loading}>
+            <LoadingBoundary loading={loadingModuleData}>
               <HTTPAccessFallbackBoundary
                 notFound={notFound}
                 forbidden={forbidden}
                 unauthorized={unauthorized}
               >
                 <RedirectBoundary>
                   <InnerLayoutRouter
-                    parallelRouterKey={parallelRouterKey}
                     url={url}
                     tree={tree}
-                    childNodes={childNodesForParallelRouter!}
+                    cacheNode={cacheNode}
                     segmentPath={segmentPath}
-                    cacheKey={cacheKey}
                   />
                 </RedirectBoundary>
               </HTTPAccessFallbackBoundary>

diff --git 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
@@ -146,10 +146,9 @@ export const AppRouterContext = React.createContext<AppRouterInstance | null>(
   null
 )
 export const LayoutRouterContext = React.createContext<{
-  childNodes: CacheNode['parallelRoutes']
-  tree: FlightRouterState
+  parentTree: FlightRouterState
+  parentCacheNode: CacheNode
   url: string
-  loading: LoadingModuleData | Promise<LoadingModuleData>
 } | 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
@@ -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 <table>. Make sure you don't have any extra whitespace between tags on each line of your source code.
-      This will cause a hydration error.
-
-        ...
-          <RenderFromTemplateContext>
-            <ScrollAndFocusHandler segmentPath={[...]}>
-              <InnerScrollAndFocusHandler segmentPath={[...]} focusAndScrollRef={{apply:false, ...}}>
-                <ErrorBoundary errorComponent={undefined} errorStyles={undefined} errorScripts={undefined}>
-                  <LoadingBoundary loading={null}>
-                    <HTTPAccessFallbackBoundary notFound={[...]} forbidden={undefined} unauthorized={undefined}>
-                      <HTTPAccessFallbackErrorBoundary pathname="/" notFound={[...]} forbidden={undefined} ...>
-                        <RedirectBoundary>
-                          <RedirectErrorBoundary router={{...}}>
-                            <InnerLayoutRouter parallelRouterKey="children" url="/" tree={[...]} childNodes={Map} ...>
-                              <ClientPageRoot Component={function Page} searchParams={{}} params={{}}>
-                                <Page params={Promise} searchParams={Promise}>
-      >                           <table>
-      >                             {" "}
-                                    ...
+"In HTML, whitespace text nodes cannot be a child of <table>. Make sure you don't have any extra whitespace between tags on each line of your source code.
+This will cause a hydration error.
+
+  ...
+    <RenderFromTemplateContext>
+      <ScrollAndFocusHandler segmentPath={[...]}>
+        <InnerScrollAndFocusHandler segmentPath={[...]} focusAndScrollRef={{apply:false, ...}}>
+          <ErrorBoundary errorComponent={undefined} errorStyles={undefined} errorScripts={undefined}>
+            <LoadingBoundary loading={null}>
+              <HTTPAccessFallbackBoundary notFound={[...]} forbidden={undefined} unauthorized={undefined}>
+                <HTTPAccessFallbackErrorBoundary pathname="/" notFound={[...]} forbidden={undefined} ...>
+                  <RedirectBoundary>
+                    <RedirectErrorBoundary router={{...}}>
+                      <InnerLayoutRouter url="/" tree={[...]} cacheNode={{lazyData:null, ...}} segmentPath={[...]}>
+                        <ClientPageRoot Component={function Page} searchParams={{}} params={{}}>
+                          <Page params={Promise} searchParams={Promise}>
+>                           <table>
+>                             {" "}
                               ...
-      "
-    `)
+                        ...
+"
+`)
 
     // 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(`
-        "...
-          <HTTPAccessFallbackErrorBoundary pathname="/" notFound={[...]} forbidden={undefined} unauthorized={undefined} ...>
-            <RedirectBoundary>
-              <RedirectErrorBoundary router={{...}}>
-                <InnerLayoutRouter parallelRouterKey="children" url="/" tree={[...]} childNodes={Map} segmentPath={[...]} ...>
-                  <ClientPageRoot Component={function Page} searchParams={{}} params={{}}>
-                    <Page params={Promise} searchParams={Promise}>
-                      <div>
-                        <div>
-                          <div>
-                            <div>
-                              <Mismatch>
-                                <p>
-                                  <span>
-                                    ...
-        +                            client
-        -                            server"
-      `)
+"...
+  <HTTPAccessFallbackErrorBoundary pathname="/" notFound={[...]} forbidden={undefined} unauthorized={undefined} ...>
+    <RedirectBoundary>
+      <RedirectErrorBoundary router={{...}}>
+        <InnerLayoutRouter url="/" tree={[...]} cacheNode={{lazyData:null, ...}} segmentPath={[...]}>
+          <ClientPageRoot Component={function Page} searchParams={{}} params={{}}>
+            <Page params={Promise} searchParams={Promise}>
+              <div>
+                <div>
+                  <div>
+                    <div>
+                      <Mismatch>
+                        <p>
+                          <span>
+                            ...
++                            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
@@ -77,7 +77,7 @@ describe('Dynamic IO Dev Errors', () => {
             '\n    at html (<anonymous>)' +
             '\n    at Root [Server] (<anonymous>)'
           : // 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)