fix(gatsby): don't show error for proper redirects (#19789)

* Created isPageExist method in loader.js, added e2e tests for redirect feature

* make function dev-only and simplify logic

* e2e: move to navigation directory and spy on console.error to validate

* remove comment from copy pasting

* rename method to mark this is not public API and shouldn't be used (we do pass loader to browser APIs)

* remove debug code

* more removals

* test case of 404 not presetnt as well

* apparently some changes we did during resilient resource loading made this work properly regardless if 404 is present

* fixup - remove commented out export

Co-authored-by: Michal Piechowiak <misiek.piechowiak@gmail.com>

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

@@ -0,0 +1,98 @@
+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`).waitForRouteChange()
+
+    cy.location(`pathname`).should(`equal`, `/`)
+    cy.then(() => {
+      const calls = spy.getCalls()
+
+      const callsAboutRedirectMatchingPage = calls.filter(call => {
+        return 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`, () => {
+    cy.visit(`/redirect`).waitForRouteChange()
+
+    cy.location(`pathname`).should(`equal`, `/`)
+    cy.then(() => {
+      const calls = spy.getCalls()
+
+      const callsAboutRedirectMatchingPage = calls.filter(call => {
+        return 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.`
+      )
+    })
+  })
+}
+
+describe(`redirect`, () => {
+  describe("404 is present", () => {
+    before(() => {
+      cy.task(`restoreAllBlockedResources`)
+    })
+
+    // this is sanity check for this group
+    it(`make sure 404 is present`, () => {
+      cy.visit(`/______not_existing_page`).waitForRouteChange()
+      cy.queryByText("Preview custom 404 page").click()
+      cy.queryByText("A custom 404 page wasn't detected", {
+        exact: false,
+      }).should(`not.exist`)
+      cy.queryByText(
+        "You just hit a route that does not exist... the sadness."
+      ).should(`exist`)
+    })
+
+    runTests()
+  })
+
+  describe("no 404", () => {
+    before(() => {
+      cy.task(`restoreAllBlockedResources`)
+
+      cy.task(`blockAssetsForPage`, {
+        pagePath: `/404`,
+        filter: `page-data`,
+      })
+      cy.task(`blockAssetsForPage`, {
+        pagePath: `/404.html`,
+        filter: `page-data`,
+      })
+    })
+
+    after(() => {
+      cy.task(`restoreAllBlockedResources`)
+    })
+
+    it(`make sure 404 is NOT present`, () => {
+      cy.visit(`/______not_existing_page`).waitForRouteChange()
+      cy.queryByText("Preview custom 404 page").click()
+      cy.queryByText("A custom 404 page wasn't detected", {
+        exact: false,
+      }).should(`exist`)
+      cy.queryByText(
+        "You just hit a route that does not exist... the sadness.",
+        { exact: false }
+      ).should(`not.exist`)
+    })
+
+    runTests()
+  })
+})

e2e-tests/development-runtime/cypress/plugins/block-resources.js

@@ -0,0 +1,66 @@
+// this is partial copy of e2e-tests/production-runtime/cypress/plugins/block-resources.js
+// partial because there is no asset blocking possibly in develop (we have single, virtual development bundle)
+
+const fs = require(`fs-extra`)
+const path = require(`path`)
+const glob = require(`glob`)
+
+const publicDir = path.join(__dirname, `..`, `..`, `public`)
+
+const moveAsset = (from, to) => {
+  const fromExists = fs.existsSync(from)
+  const toExists = fs.existsSync(to)
+
+  if (fromExists && !toExists) {
+    fs.moveSync(from, to, {
+      overwrite: true,
+    })
+  }
+}
+
+const restorePageData = hiddenPath => {
+  if (path.basename(hiddenPath).charAt(0) !== `_`) {
+    throw new Error(`hiddenPath should have _ prefix`)
+  }
+  const restoredPath = path.join(
+    path.dirname(hiddenPath),
+    path.basename(hiddenPath).slice(1)
+  )
+  moveAsset(hiddenPath, restoredPath)
+}
+
+const getPageDataPath = pagePath => {
+  const fixedPagePath = pagePath === `/` ? `index` : pagePath
+  return path.join(publicDir, `page-data`, fixedPagePath, `page-data.json`)
+}
+
+const getHiddenPageDataPath = pagePath => {
+  const fixedPagePath = pagePath === `/` ? `index` : pagePath
+  return path.join(publicDir, `page-data`, fixedPagePath, `_page-data.json`)
+}
+
+const blockPageData = pagePath =>
+  moveAsset(getPageDataPath(pagePath), getHiddenPageDataPath(pagePath))
+
+const blockAssetsForPage = ({ pagePath, filter }) => {
+  if (filter === `all` || filter === `page-data`) {
+    blockPageData(pagePath)
+  }
+
+  console.log(`Blocked assets for path "${pagePath}" [${filter}]`)
+  return null
+}
+
+const restore = () => {
+  const globPattern = path.join(publicDir, `/page-data/**`, `_page-data.json`)
+  const hiddenPageDatas = glob.sync(globPattern)
+  hiddenPageDatas.forEach(restorePageData)
+
+  console.log(`Restored resources`)
+  return null
+}
+
+module.exports = {
+  restoreAllBlockedResources: restore,
+  blockAssetsForPage,
+}

e2e-tests/development-runtime/cypress/plugins/index.js

@@ -10,8 +10,10 @@
 
 // This function is called when a project is opened or re-opened (e.g. due to
 // the project's config changing)
+const blockResources = require(`./block-resources`)
 
 module.exports = (on, config) => {
   // `on` is used to hook into various events Cypress emits
   // `config` is the resolved Cypress config
+  on(`task`, Object.assign({}, blockResources))
 }

e2e-tests/development-runtime/gatsby-node.js

@@ -28,7 +28,7 @@ exports.onCreateNode = function onCreateNode({
 }
 
 exports.createPages = async function createPages({
-  actions: { createPage },
+  actions: { createPage, createRedirect },
   graphql,
 }) {
   const { data } = await graphql(`
@@ -87,6 +87,20 @@ exports.createPages = async function createPages({
     path: `/client-only-paths/static`,
     component: path.resolve(`src/templates/static-page.js`),
   })
+
+  createRedirect({
+    fromPath: `/redirect-without-page`,
+    toPath: `/`,
+    isPermanent: true,
+    redirectInBrowser: true,
+  })
+
+  createRedirect({
+    fromPath: `/redirect`,
+    toPath: `/`,
+    isPermanent: true,
+    redirectInBrowser: true,
+  })
 }
 
 exports.onCreatePage = async ({ page, actions }) => {

e2e-tests/development-runtime/src/pages/redirect.js

@@ -0,0 +1,13 @@
+import React from "react"
+
+import Layout from "../components/layout"
+import SEO from "../components/seo"
+
+const Redirect = () => (
+  <Layout>
+    <SEO title="Redirect" />
+    <p>Redirecting...</p>
+  </Layout>
+)
+
+export default Redirect

packages/gatsby/cache-dir/navigation.js

@@ -20,9 +20,7 @@ function maybeRedirect(pathname) {
 
   if (redirect != null) {
     if (process.env.NODE_ENV !== `production`) {
-      const pageResources = loader.loadPageSync(pathname)
-
-      if (pageResources != null) {
+      if (!loader.isPageNotFound(pathname)) {
         console.error(
           `The route "${pathname}" matches both a page and a redirect; this is probably not intentional.`
         )