fix(gatsby): Wrong route resolved by findPageByPath function (#34070)

* use best matching route * add more comments * add tests * add matchPath specifity tests to findPageByPath * add note about possible optimalization for matchPath handling Co-authored-by: Michal Piechowiak <misiek.piechowiak@gmail.com>
gatsbyjs · Jan 4, 2022 · 0cc5a5a · 0cc5a5a
1 parent 33049c8
commit 0cc5a5a
Show file tree

Hide file tree

Showing 6 changed files with 199 additions and 8 deletions.
diff --git a/e2e-tests/production-runtime/cypress/integration/ssr.js b/e2e-tests/production-runtime/cypress/integration/ssr.js
@@ -1,6 +1,7 @@
 const staticPath = `/ssr/static-path/`
 const paramPath = `/ssr/param-path/`
 const wildcardPath = `/ssr/wildcard-path/`
+const pathRaking = `/ssr/path-ranking/`
 
 describe(`Static path ('${staticPath}')`, () => {
   it(`Direct visit no query params`, () => {
@@ -72,6 +73,32 @@ describe(`Param path ('${paramPath}:param')`, () => {
   })
 })
 
+describe(`Path ranking resolution ('${pathRaking}*')`, () => {
+  it(`Resolves to [...].js template at ${pathRaking}p1`, () => {
+    cy.visit(pathRaking + `p1/`).waitForRouteChange()
+    cy.getTestElement(`query`).contains(`{}`)
+    cy.getTestElement(`params`).contains(`{"*":"p1"}`)
+  })
+
+  it(`Resolves to [p1]/[p2].js template at ${pathRaking}p1/p2`, () => {
+    cy.visit(pathRaking + `p1/p2/`).waitForRouteChange()
+    cy.getTestElement(`query`).contains(`{}`)
+    cy.getTestElement(`params`).contains(`{"p1":"p1","p2":"p2"}`)
+  })
+
+  it(`Resolves to [p1]/page.js template at ${pathRaking}p1/page`, () => {
+    cy.visit(pathRaking + `p1/page/`).waitForRouteChange()
+    cy.getTestElement(`query`).contains(`{}`)
+    cy.getTestElement(`params`).contains(`{"p1":"p1"}`)
+  })
+
+  it(`Resolves to [...].js template at ${pathRaking}p1/p2/p3`, () => {
+    cy.visit(pathRaking + `p1/p2/p3/`).waitForRouteChange()
+    cy.getTestElement(`query`).contains(`{}`)
+    cy.getTestElement(`params`).contains(`{"*":"p1/p2/p3"}`)
+  })
+})
+
 describe(`Wildcard path ('${wildcardPath}*')`, () => {
   it(`Direct visit no query params`, () => {
     cy.visit(wildcardPath + `foo/nested/`).waitForRouteChange()

diff --git a/e2e-tests/production-runtime/src/pages/ssr/path-ranking/[...].js b/e2e-tests/production-runtime/src/pages/ssr/path-ranking/[...].js
@@ -0,0 +1,22 @@
+import React from "react"
+
+export default function StaticPath({ serverData }) {
+  return (
+    <div>
+      <h2>Query</h2>
+      <pre data-testid="query">{JSON.stringify(serverData?.arg?.query)}</pre>
+      <h2>Params</h2>
+      <pre data-testid="params">{JSON.stringify(serverData?.arg?.params)}</pre>
+      <h2>Debug</h2>
+      <pre>{JSON.stringify({ serverData }, null, 2)}</pre>
+    </div>
+  )
+}
+
+export function getServerData(arg) {
+  return {
+    props: {
+      arg,
+    },
+  }
+}
diff --git a/e2e-tests/production-runtime/src/pages/ssr/path-ranking/[p1]/[p2].js b/e2e-tests/production-runtime/src/pages/ssr/path-ranking/[p1]/[p2].js
@@ -0,0 +1,22 @@
+import React from "react"
+
+export default function StaticPath({ serverData }) {
+  return (
+    <div>
+      <h2>Query</h2>
+      <pre data-testid="query">{JSON.stringify(serverData?.arg?.query)}</pre>
+      <h2>Params</h2>
+      <pre data-testid="params">{JSON.stringify(serverData?.arg?.params)}</pre>
+      <h2>Debug</h2>
+      <pre>{JSON.stringify({ serverData }, null, 2)}</pre>
+    </div>
+  )
+}
+
+export function getServerData(arg) {
+  return {
+    props: {
+      arg,
+    },
+  }
+}
diff --git a/e2e-tests/production-runtime/src/pages/ssr/path-ranking/[p1]/page.js b/e2e-tests/production-runtime/src/pages/ssr/path-ranking/[p1]/page.js
@@ -0,0 +1,22 @@
+import React from "react"
+
+export default function StaticPath({ serverData }) {
+  return (
+    <div>
+      <h2>Query</h2>
+      <pre data-testid="query">{JSON.stringify(serverData?.arg?.query)}</pre>
+      <h2>Params</h2>
+      <pre data-testid="params">{JSON.stringify(serverData?.arg?.params)}</pre>
+      <h2>Debug</h2>
+      <pre>{JSON.stringify({ serverData }, null, 2)}</pre>
+    </div>
+  )
+}
+
+export function getServerData(arg) {
+  return {
+    props: {
+      arg,
+    },
+  }
+}
diff --git a/packages/gatsby/src/utils/__tests__/find-page-by-path.ts b/packages/gatsby/src/utils/__tests__/find-page-by-path.ts
@@ -41,7 +41,22 @@ const commonPages = [
     path: `/app/`,
     matchPath: `/app/*`,
   },
+  {
+    path: `/app/p1/page/`,
+    matchPath: `/app/:p1/page`,
+  },
+  {
+    path: `/app/p1/p2/`,
+    matchPath: `/app/:p1/:p2`,
+  },
+  {
+    path: `/app/p1/page2/`,
+    // this is very similar to `/app/:p1/page`, point of adding 2 of those is to make sure order of pages in state
+    // doesn't impact deterministic page selection
+    matchPath: `/app/:p1/page2`,
+  },
   `/app/static/`,
+  `/app/static/page/`,
 ]
 
 const state = generatePagesState([...commonPages])
@@ -121,16 +136,56 @@ describe(`findPageByPath`, () => {
       expect(page?.path).toEqual(`/app/`)
     })
 
+    it(`Picks most specific matchPath`, () => {
+      {
+        const page = findPageByPath(state, `/app/foo`)
+        expect(page).toBeDefined()
+        expect(page?.path).toEqual(`/app/`)
+      }
+
+      {
+        const page = findPageByPath(state, `/app/foo/bar/baz`)
+        expect(page).toBeDefined()
+        expect(page?.path).toEqual(`/app/`)
+      }
+
+      {
+        const page = findPageByPath(state, `/app/foo/bar`)
+        expect(page).toBeDefined()
+        expect(page?.path).toEqual(`/app/p1/p2/`)
+      }
+
+      {
+        const page = findPageByPath(state, `/app/foo/page`)
+        expect(page).toBeDefined()
+        expect(page?.path).toEqual(`/app/p1/page/`)
+      }
+
+      {
+        const page = findPageByPath(state, `/app/foo/page2`)
+        expect(page).toBeDefined()
+        expect(page?.path).toEqual(`/app/p1/page2/`)
+      }
+    })
+
     it(`Can match client-only path by static`, () => {
       const page = findPageByPath(state, `/app`)
       expect(page).toBeDefined()
       expect(page?.path).toEqual(`/app/`)
     })
 
     it(`Will prefer static page over client-only in case both match`, () => {
-      const page = findPageByPath(state, `/app/static`)
-      expect(page).toBeDefined()
-      expect(page?.path).toEqual(`/app/static/`)
+      {
+        const page = findPageByPath(state, `/app/static`)
+        expect(page).toBeDefined()
+        expect(page?.path).toEqual(`/app/static/`)
+      }
+
+      {
+        const page = findPageByPath(state, `/app/static/page`)
+        expect(page).toBeDefined()
+        expect(page?.path).toEqual(`/app/static/page/`)
+      }
     })
   })
 

diff --git a/packages/gatsby/src/utils/find-page-by-path.ts b/packages/gatsby/src/utils/find-page-by-path.ts
@@ -1,5 +1,44 @@
 import { IGatsbyPage, IGatsbyState } from "../redux/types"
-import { match } from "@gatsbyjs/reach-router/lib/utils"
+import { pick } from "@gatsbyjs/reach-router/lib/utils"
+
+// Ranks and picks the best page to match. Each segment gets the highest
+// amount of points, then the type of segment gets an additional amount of
+// points where
+//
+//     static > dynamic > splat > root
+//
+// This way we don't have to worry about the order of our pages, let the
+// computers do it.
+//
+// In the future, we could move this pagesByMatchPath computation outside this
+// function and save some processing power
+const findBestMatchingPage = (
+  pages: Map<string, IGatsbyPage>,
+  path: string
+): IGatsbyPage | null => {
+  // Pick only routes with matchPath for better performance.
+  // Exact match should have already been checked
+  const pagesByMatchPath: Record<string, IGatsbyPage> = {}
+  for (const page of pages.values()) {
+    const matchPath = page.matchPath
+    if (matchPath) {
+      pagesByMatchPath[matchPath] = page
+    }
+  }
+
+  const routes = Object.keys(pagesByMatchPath).map(path => {
+    return { path }
+  })
+
+  // picks best matching route with reach router's algorithm
+  const picked = pick(routes, path)
+
+  if (picked) {
+    return pagesByMatchPath[picked.route.path]
+  }
+
+  return null
+}
 
 export function findPageByPath(
   state: IGatsbyState,
@@ -46,10 +85,14 @@ export function findPageByPath(
   }
 
   // we didn't find exact static page, time to check matchPaths
-  for (const page of pages.values()) {
-    if (page.matchPath && match(page.matchPath, path)) {
-      return page
-    }
+  // TODO: consider using `match-paths.json` generated by `requires-writer`
+  // to avoid looping through all pages again. Ideally generate smaller `match-paths.json`
+  // variant that doesn't including overlapping static pages in `requires-writer` as well
+  // as this function already checked static paths at this point
+  const matchingPage = findBestMatchingPage(pages, path)
+
+  if (matchingPage) {
+    return matchingPage
   }
 
   if (fallbackTo404) {