fix(gatsby): preload and prefetches check for in-browser redirects (#31366)

* fix(gatsby): preload and prefetches check for in-browser redirects

* maybe json and js should not be named the same

* copy json files to commonjs dir

* add e2e test case

* move checking for page and redirect overlap to node from client

* adjust e2e assertions

Author: pieh

e2e-tests/development-runtime/cypress/integration/navigation/redirect.js

@@ -1,26 +1,10 @@
-let spy
-Cypress.on(`window:before:load`, win => {
-  spy = cy.spy(win.console, `error`).as(`spyWinConsoleError`)
-})
-
 const runTests = () => {
   it(`should redirect page to index page when there is no such page`, () => {
     cy.visit(`/redirect-without-page`, {
       failOnStatusCode: false,
     }).waitForRouteChange()
 
     cy.location(`pathname`).should(`equal`, `/`)
-    cy.then(() => {
-      const calls = spy.getCalls()
-
-      const callsAboutRedirectMatchingPage = calls.filter(call =>
-        call.args[0].includes(
-          `matches both a page and a redirect; this is probably not intentional.`
-        )
-      )
-
-      expect(callsAboutRedirectMatchingPage.length).to.equal(0)
-    })
   })
 
   it(`should redirect page to index page even there is a such page`, () => {
@@ -29,20 +13,6 @@ const runTests = () => {
     }).waitForRouteChange()
 
     cy.location(`pathname`).should(`equal`, `/`)
-    cy.then(() => {
-      const calls = spy.getCalls()
-
-      const callsAboutRedirectMatchingPage = calls.filter(call =>
-        call.args[0].includes(
-          `matches both a page and a redirect; this is probably not intentional.`
-        )
-      )
-
-      expect(callsAboutRedirectMatchingPage.length).not.to.equal(0)
-      expect(spy).to.be.calledWith(
-        `The route "/redirect" matches both a page and a redirect; this is probably not intentional.`
-      )
-    })
   })
 
   it(`should redirect to a dynamically-created replacement page`, () => {
@@ -51,15 +21,6 @@ const runTests = () => {
     }).waitForRouteChange()
 
     cy.location(`pathname`).should(`equal`, `/pt/redirect-me/`)
-    cy.then(() => {
-      expect(spy).not.to.be.calledWith(
-        `The route "/redirect" matches both a page and a redirect; this is probably not intentional.`
-      )
-
-      cy.findByText(`This should be at /pt/redirect-me/`, {
-        exact: false,
-      }).should(`exist`)
-    })
   })
 }
 

e2e-tests/production-runtime/cypress/integration/redirects.js

@@ -1,3 +1,8 @@
+let spy
+Cypress.on(`window:before:load`, win => {
+  spy = cy.spy(win.console, `error`).as(`spyWinConsoleError`)
+})
+
 describe(`Redirects`, () => {
   it(`are case insensitive when ignoreCase is set to true`, () => {
     cy.visit(`/Longue-PAGE`, { failOnStatusCode: false }).waitForRouteChange()
@@ -9,4 +14,47 @@ describe(`Redirects`, () => {
 
     cy.get(`h1`).invoke(`text`).should(`contain`, `NOT FOUND`)
   })
+
+  it(`use redirects when preloading page-data`, () => {
+    const expectedLinks = [`/Longue-PAGE`, `/pagina-larga`]
+
+    // we should not hit those routes
+    cy.intercept("GET", "/page-data/Longue-PAGE/page-data.json").as(
+      "page-data-for-redirected-page-a"
+    )
+    cy.intercept("GET", "/page-data/pagina-larga/page-data.json").as(
+      "page-data-for-redirected-page-b"
+    )
+
+    cy.intercept("GET", "/page-data/long-page/page-data.json").as(
+      "redirected-page-data"
+    )
+
+    cy.visit(`/redirect-links/`).waitForRouteChange()
+
+    cy.get("a").each(($el, index, $list) => {
+      cy.then(() => {
+        expect($el[0].href.replace(`http://localhost:9000`, ``)).to.be.oneOf(
+          expectedLinks
+        )
+      })
+      // focus / hover links to force trigger preload
+      cy.wrap($el).trigger("mouseover")
+    })
+
+    cy.then(() => {
+      // those requests should not happen
+      cy.get("@page-data-for-redirected-page-a").should(networkCall => {
+        expect(networkCall).to.be.null
+      })
+      cy.get("@page-data-for-redirected-page-b").should(networkCall => {
+        expect(networkCall).to.be.null
+      })
+
+      // instead we want links to use redirects
+      cy.get("@redirected-page-data").should(networkCall => {
+        expect(networkCall.response.statusCode).to.be.oneOf([304, 200])
+      })
+    })
+  })
 })

e2e-tests/production-runtime/src/pages/redirect-links.js

@@ -0,0 +1,15 @@
+import * as React from "react"
+import { Link } from "gatsby"
+
+export default function RedirectLinks() {
+  return (
+    <ul>
+      <li>
+        <Link to="/Longue-PAGE">/Longue-PAGE</Link>
+      </li>
+      <li>
+        <Link to="/pagina-larga">/pagina-larga</Link>
+      </li>
+    </ul>
+  )
+}

packages/gatsby/cache-dir/find-path.js

@@ -1,6 +1,7 @@
 import { pick } from "@gatsbyjs/reach-router/lib/utils"
 import stripPrefix from "./strip-prefix"
 import normalizePagePath from "./normalize-page-path"
+import { maybeGetBrowserRedirect } from "./redirect-utils.js"
 
 const pathCache = new Map()
 let matchPaths = []
@@ -115,6 +116,11 @@ export const findPath = rawPathname => {
     return pathCache.get(trimmedPathname)
   }
 
+  const redirect = maybeGetBrowserRedirect(rawPathname)
+  if (redirect) {
+    return findPath(redirect.toPath)
+  }
+
   let foundPath = findMatchPath(trimmedPathname)
 
   if (!foundPath) {

packages/gatsby/cache-dir/navigation.js

@@ -1,42 +1,18 @@
 import React from "react"
 import PropTypes from "prop-types"
 import loader, { PageResourceStatus } from "./loader"
-import redirects from "./redirects.json"
+import { maybeGetBrowserRedirect } from "./redirect-utils.js"
 import { apiRunner } from "./api-runner-browser"
 import emitter from "./emitter"
 import { RouteAnnouncerProps } from "./route-announcer-props"
 import { navigate as reachNavigate } from "@gatsbyjs/reach-router"
 import { globalHistory } from "@gatsbyjs/reach-router/lib/history"
 import { parsePath } from "gatsby-link"
 
-// Convert to a map for faster lookup in maybeRedirect()
-
-const redirectMap = new Map()
-const redirectIgnoreCaseMap = new Map()
-
-redirects.forEach(redirect => {
-  if (redirect.ignoreCase) {
-    redirectIgnoreCaseMap.set(redirect.fromPath, redirect)
-  } else {
-    redirectMap.set(redirect.fromPath, redirect)
-  }
-})
-
 function maybeRedirect(pathname) {
-  let redirect = redirectMap.get(pathname)
-  if (!redirect) {
-    redirect = redirectIgnoreCaseMap.get(pathname.toLowerCase())
-  }
+  const redirect = maybeGetBrowserRedirect(pathname)
 
   if (redirect != null) {
-    if (process.env.NODE_ENV !== `production`) {
-      if (!loader.isPageNotFound(pathname)) {
-        console.error(
-          `The route "${pathname}" matches both a page and a redirect; this is probably not intentional.`
-        )
-      }
-    }
-
     window.___replace(redirect.toPath)
     return true
   } else {
@@ -72,10 +48,7 @@ const navigate = (to, options = {}) => {
   }
 
   let { pathname } = parsePath(to)
-  let redirect = redirectMap.get(pathname)
-  if (!redirect) {
-    redirect = redirectIgnoreCaseMap.get(pathname.toLowerCase())
-  }
+  const redirect = maybeGetBrowserRedirect(pathname)
 
   // If we're redirecting, just replace the passed in pathname
   // to the one we want to redirect to.
@@ -270,4 +243,4 @@ RouteUpdates.propTypes = {
   location: PropTypes.object.isRequired,
 }
 
-export { init, shouldUpdateScroll, RouteUpdates }
+export { init, shouldUpdateScroll, RouteUpdates, maybeGetBrowserRedirect }

packages/gatsby/cache-dir/redirect-utils.js

@@ -0,0 +1,22 @@
+import redirects from "./redirects.json"
+
+// Convert to a map for faster lookup in maybeRedirect()
+
+const redirectMap = new Map()
+const redirectIgnoreCaseMap = new Map()
+
+redirects.forEach(redirect => {
+  if (redirect.ignoreCase) {
+    redirectIgnoreCaseMap.set(redirect.fromPath, redirect)
+  } else {
+    redirectMap.set(redirect.fromPath, redirect)
+  }
+})
+
+export function maybeGetBrowserRedirect(pathname) {
+  let redirect = redirectMap.get(pathname)
+  if (!redirect) {
+    redirect = redirectIgnoreCaseMap.get(pathname.toLowerCase())
+  }
+  return redirect
+}

packages/gatsby/package.json

@@ -244,7 +244,7 @@
     "postbuild": "node scripts/output-api-file.js && yarn workspace gatsby-admin build",
     "build:internal-plugins": "copyfiles -u 1 src/internal-plugins/**/package.json dist",
     "build:rawfiles": "copyfiles -u 1 src/internal-plugins/**/raw_* dist",
-    "build:cjs": "babel cache-dir --out-dir cache-dir/commonjs --ignore \"**/__tests__\" --ignore \"**/__mocks__\"",
+    "build:cjs": "babel cache-dir --out-dir cache-dir/commonjs --ignore \"**/__tests__\" --ignore \"**/__mocks__\" && copyfiles -u 1 cache-dir/**/*.json cache-dir/commonjs",
     "build:src": "babel src --out-dir dist --source-maps --verbose --ignore \"**/gatsby-cli.js,src/internal-plugins/dev-404-page/raw_dev-404-page.js,**/__tests__,**/__mocks__\" --extensions \".ts,.js\"",
     "build:types": "tsc --emitDeclarationOnly --declaration --declarationDir dist",
     "clean-test-bundles": "find test/ -type f -name bundle.js* -exec rm -rf {} +",

packages/gatsby/src/bootstrap/__tests__/redirects-writer.ts

@@ -0,0 +1,116 @@
+import { writeRedirects } from "../redirects-writer"
+import * as fs from "fs-extra"
+import reporter from "gatsby-cli/lib/reporter"
+import { store } from "../../redux"
+import { actions } from "../../redux/actions"
+
+jest.mock(`fs-extra`, () => {
+  return { writeFile: jest.fn() }
+})
+
+jest.mock(`gatsby-cli/lib/reporter`, () => {
+  return {
+    warn: jest.fn(),
+  }
+})
+
+const writeFileMock = fs.writeFile as jest.MockedFunction<typeof fs.writeFile>
+const reporterWarnMock = reporter.warn as jest.MockedFunction<
+  typeof reporter.warn
+>
+
+beforeEach(() => {
+  writeFileMock.mockClear()
+  reporterWarnMock.mockClear()
+  store.dispatch({ type: `DELETE_CACHE` })
+})
+
+describe(`redirect-writer`, () => {
+  it(`doesn't save non browser redirects`, async () => {
+    store.dispatch(
+      actions.createRedirect({
+        fromPath: `/server/`,
+        toPath: `/server/redirect/`,
+      })
+    )
+    store.dispatch(
+      actions.createRedirect({
+        fromPath: `/client/`,
+        toPath: `/client/redirect/`,
+        redirectInBrowser: true,
+      })
+    )
+
+    await writeRedirects()
+
+    expect(writeFileMock).toBeCalledTimes(1)
+
+    const clientSideRedirects = JSON.parse(writeFileMock.mock.calls[0][1])
+
+    expect(clientSideRedirects).toContainEqual(
+      expect.objectContaining({ fromPath: `/client/` })
+    )
+    expect(clientSideRedirects).not.toContainEqual(
+      expect.objectContaining({ fromPath: `/server/` })
+    )
+
+    expect(reporterWarnMock).not.toBeCalled()
+  })
+
+  it(`show warning when there is both redirect and page with same path`, async () => {
+    store.dispatch(
+      actions.createRedirect({
+        fromPath: `/server-overlap/`,
+        toPath: `/server-overlap/redirect/`,
+      })
+    )
+    store.dispatch(
+      actions.createRedirect({
+        fromPath: `/client-overlap`, // intentionally missing trailing slash - this checks if /client-overlap/ page is discovered
+        toPath: `/client-overlap/redirect/`,
+        redirectInBrowser: true,
+      })
+    )
+    store.dispatch(
+      actions.createPage(
+        {
+          path: `/server-overlap`, // intentionally missing trailing slash - this checks if redirect for /server-overlap/ is discovered
+          component: `/whatever/index.js`,
+        },
+        { id: `test`, name: `test` }
+      )
+    )
+    store.dispatch(
+      actions.createPage(
+        {
+          path: `/client-overlap/`,
+          component: `/whatever/index.js`,
+        },
+        { id: `test`, name: `test` }
+      )
+    )
+
+    await writeRedirects()
+
+    expect(writeFileMock).toBeCalledTimes(1)
+
+    const clientSideRedirects = JSON.parse(writeFileMock.mock.calls[0][1])
+
+    expect(clientSideRedirects).toContainEqual(
+      expect.objectContaining({ fromPath: `/client-overlap` })
+    )
+
+    expect(clientSideRedirects).not.toContainEqual(
+      expect.objectContaining({ fromPath: `/server-overlap/` })
+    )
+
+    expect(reporterWarnMock).toBeCalledTimes(1)
+
+    const warningMessage = reporterWarnMock.mock.calls[0][0]
+    expect(warningMessage).toMatchInlineSnapshot(`
+      "There are routes that match both page and redirect. It will result in page not being accessible; this is probably not intentional:
+       - page: \\"/server-overlap\\" and redirect: \\"/server-overlap/\\" -> \\"/server-overlap/redirect/\\"
+       - page: \\"/client-overlap/\\" and redirect: \\"/client-overlap\\" -> \\"/client-overlap/redirect/\\""
+    `)
+  })
+})