feat(gatsby): Add eslint rules to warn against bad patterns in pageTe…

…mplates (for Fast Refresh) (#28689)

Co-authored-by: LekoArts <lekoarts@gmail.com>
Co-authored-by: Peter van der Zee <209817+pvdz@users.noreply.github.com>
Co-authored-by: Michal Piechowiak <misiek.piechowiak@gmail.com>
Co-authored-by: gatsbybot <mathews.kyle+gatsbybot@gmail.com>

e2e-tests/development-runtime/cypress/integration/eslint-rules/limited-exports-page-templates.js

@@ -0,0 +1,14 @@
+if (Cypress.env("HOT_LOADER") === `fast-refresh`) {
+  describe(`limited-exports-page-templates`, () => {
+    it(`should log warning to console for invalid export`, () => {
+      cy.visit(`/eslint-rules/limited-exports-page-templates`, {
+        onBeforeLoad(win) {
+          cy.stub(win.console, 'log').as(`consoleLog`)
+        }
+      }).waitForRouteChange()
+
+      cy.get(`@consoleLog`).should(`be.calledWithMatch`, /15:1  warning  In page templates only a default export of a valid React component and the named export of a page query is allowed./i)
+      cy.get(`@consoleLog`).should(`not.be.calledWithMatch`, /17:1  warning  In page templates only a default export of a valid React component and the named export of a page query is allowed./i)
+    })
+  })
+}

e2e-tests/development-runtime/cypress/integration/eslint-rules/no-anonymous-exports-page-templates.js

@@ -0,0 +1,22 @@
+if (Cypress.env("HOT_LOADER") === `fast-refresh`) {
+  describe(`no-anonymous-exports-page-templates`, () => {
+    it(`should log warning to console for arrow functions`, () => {
+      cy.visit(`/eslint-rules/no-anonymous-exports-page-templates`, {
+        onBeforeLoad(win) {
+          cy.stub(win.console, 'log').as(`consoleLog`)
+        }
+      }).waitForRouteChange()
+
+      cy.get(`@consoleLog`).should(`be.calledWithMatch`, /Anonymous arrow functions cause Fast Refresh to not preserve local component state./i)
+    })
+    it(`should log warning to console for function declarations`, () => {
+      cy.visit(`/eslint-rules/no-anonymous-exports-page-templates-function`, {
+        onBeforeLoad(win) {
+          cy.stub(win.console, 'log').as(`consoleLog`)
+        }
+      }).waitForRouteChange()
+
+      cy.get(`@consoleLog`).should(`be.calledWithMatch`, /Anonymous function declarations cause Fast Refresh to not preserve local component state./i)
+    })
+  })
+}

e2e-tests/development-runtime/src/pages/eslint-rules/limited-exports-page-templates.js

@@ -0,0 +1,27 @@
+import React from "react"
+import { graphql } from "gatsby"
+
+function PageQuery({ data }) {
+  return (
+    <div>
+      <h1 data-testid="title">Limited Exports Page Templates. ESLint Rule</h1>
+      <p data-testid="hot">
+        {data.site.siteMetadata.title}
+      </p>
+    </div>
+  )
+}
+
+export function notAllowed() {}
+
+export const query = graphql`
+  {
+    site {
+      siteMetadata {
+        title
+      }
+    }
+  }
+`
+
+export default PageQuery

e2e-tests/development-runtime/src/pages/eslint-rules/no-anonymous-exports-page-templates-function.js

@@ -0,0 +1,11 @@
+import React from "react"
+
+import Layout from "../../components/layout"
+
+export default function () {
+  return (
+    <Layout>
+      <h1 data-testid="title">Anonymous Arrow Function. ESLint Rule</h1>
+    </Layout>
+  )
+}

e2e-tests/development-runtime/src/pages/eslint-rules/no-anonymous-exports-page-templates.js

@@ -0,0 +1,9 @@
+import React from "react"
+
+import Layout from "../../components/layout"
+
+export default () => (
+  <Layout>
+    <h1 data-testid="title">Anonymous Arrow Function. ESLint Rule</h1>
+  </Layout>
+)

packages/gatsby/src/utils/__tests__/eslint-config.ts

@@ -0,0 +1,130 @@
+import { mergeRequiredConfigIn } from "../eslint-config"
+import { CLIEngine } from "eslint"
+import * as path from "path"
+
+describe(`eslint-config`, () => {
+  describe(`mergeRequiredConfigIn`, () => {
+    it(`adds rulePaths and extends if those don't exist`, () => {
+      const conf: CLIEngine.Options = {}
+
+      mergeRequiredConfigIn(conf)
+
+      expect(conf?.baseConfig).toMatchInlineSnapshot(`
+        Object {
+          "extends": Array [
+            "<PROJECT_ROOT>/packages/gatsby/src/utils/eslint/required.js",
+          ],
+        }
+      `)
+
+      expect(conf.rulePaths).toMatchInlineSnapshot(`
+        Array [
+          "<PROJECT_ROOT>/packages/gatsby/src/utils/eslint-rules",
+        ]
+      `)
+    })
+
+    it(`adds rulePath if rulePaths exist but don't contain required rules`, () => {
+      const conf: CLIEngine.Options = {
+        rulePaths: [`test`],
+      }
+
+      mergeRequiredConfigIn(conf)
+
+      expect(conf.rulePaths).toMatchInlineSnapshot(`
+        Array [
+          "test",
+          "<PROJECT_ROOT>/packages/gatsby/src/utils/eslint-rules",
+        ]
+      `)
+    })
+
+    it(`doesn't add rulePath multiple times`, () => {
+      const conf: CLIEngine.Options = {
+        rulePaths: [path.resolve(__dirname, `../eslint-rules`), `test`],
+      }
+
+      mergeRequiredConfigIn(conf)
+
+      expect(conf.rulePaths).toMatchInlineSnapshot(`
+        Array [
+          "<PROJECT_ROOT>/packages/gatsby/src/utils/eslint-rules",
+          "test",
+        ]
+      `)
+    })
+
+    it(`adds extend if extends exist (array) but don't contain required preset`, () => {
+      const conf: CLIEngine.Options = {
+        baseConfig: {
+          extends: [`ext1`],
+        },
+      }
+
+      mergeRequiredConfigIn(conf)
+
+      expect(conf.baseConfig).toMatchInlineSnapshot(`
+        Object {
+          "extends": Array [
+            "ext1",
+            "<PROJECT_ROOT>/packages/gatsby/src/utils/eslint/required.js",
+          ],
+        }
+      `)
+    })
+
+    it(`adds extend if extends exist (string) but don't contain required preset`, () => {
+      const conf: CLIEngine.Options = {
+        baseConfig: {
+          extends: `ext1`,
+        },
+      }
+
+      mergeRequiredConfigIn(conf)
+
+      expect(conf.baseConfig).toMatchInlineSnapshot(`
+        Object {
+          "extends": Array [
+            "ext1",
+            "<PROJECT_ROOT>/packages/gatsby/src/utils/eslint/required.js",
+          ],
+        }
+      `)
+    })
+
+    it(`doesn't add extend multiple times (extends is array)`, () => {
+      const conf: CLIEngine.Options = {
+        baseConfig: {
+          extends: [require.resolve(`../eslint/required`), `ext1`],
+        },
+      }
+
+      mergeRequiredConfigIn(conf)
+
+      expect(conf.baseConfig).toMatchInlineSnapshot(`
+        Object {
+          "extends": Array [
+            "<PROJECT_ROOT>/packages/gatsby/src/utils/eslint/required.js",
+            "ext1",
+          ],
+        }
+      `)
+    })
+
+    it(`doesn't add extend multiple times (extends is string)`, () => {
+      const conf: CLIEngine.Options = {
+        baseConfig: {
+          extends: require.resolve(`../eslint/required`),
+        },
+      }
+
+      mergeRequiredConfigIn(conf)
+
+      expect(conf.baseConfig).toMatchInlineSnapshot(`
+        Object {
+          "extends": "<PROJECT_ROOT>/packages/gatsby/src/utils/eslint/required.js",
+        }
+      `)
+    })
+  })
+})

packages/gatsby/src/utils/eslint-config.ts

@@ -1,5 +1,61 @@
 import { printSchema, GraphQLSchema } from "graphql"
 import { CLIEngine } from "eslint"
+import path from "path"
+
+const eslintRulePaths = path.resolve(`${__dirname}/eslint-rules`)
+const eslintRequirePreset = require.resolve(`./eslint/required`)
+
+export const eslintRequiredConfig: CLIEngine.Options = {
+  rulePaths: [eslintRulePaths],
+  baseConfig: {
+    globals: {
+      graphql: true,
+      __PATH_PREFIX__: true,
+      __BASE_PATH__: true, // this will rarely, if ever, be used by consumers
+    },
+    extends: [eslintRequirePreset],
+  },
+}
+
+export function mergeRequiredConfigIn(
+  existingOptions: CLIEngine.Options
+): void {
+  // make sure rulePaths include our custom rules
+  if (existingOptions.rulePaths) {
+    if (
+      Array.isArray(existingOptions.rulePaths) &&
+      !existingOptions.rulePaths.includes(eslintRulePaths)
+    ) {
+      existingOptions.rulePaths.push(eslintRulePaths)
+    }
+  } else {
+    existingOptions.rulePaths = [eslintRulePaths]
+  }
+
+  // make sure we extend required preset
+  if (!existingOptions.baseConfig) {
+    existingOptions.baseConfig = {}
+  }
+
+  if (existingOptions.baseConfig.extends) {
+    if (
+      Array.isArray(existingOptions.baseConfig.extends) &&
+      !existingOptions.baseConfig.extends.includes(eslintRequirePreset)
+    ) {
+      existingOptions.baseConfig.extends.push(eslintRequirePreset)
+    } else if (
+      typeof existingOptions.baseConfig.extends === `string` &&
+      existingOptions.baseConfig.extends !== eslintRequirePreset
+    ) {
+      existingOptions.baseConfig.extends = [
+        existingOptions.baseConfig.extends,
+        eslintRequirePreset,
+      ]
+    }
+  } else {
+    existingOptions.baseConfig.extends = [eslintRequirePreset]
+  }
+}
 
 export const eslintConfig = (
   schema: GraphQLSchema,
@@ -8,13 +64,17 @@ export const eslintConfig = (
   return {
     useEslintrc: false,
     resolvePluginsRelativeTo: __dirname,
+    rulePaths: [eslintRulePaths],
     baseConfig: {
       globals: {
         graphql: true,
         __PATH_PREFIX__: true,
         __BASE_PATH__: true, // this will rarely, if ever, be used by consumers
       },
-      extends: [require.resolve(`eslint-config-react-app`)],
+      extends: [
+        require.resolve(`eslint-config-react-app`),
+        eslintRequirePreset,
+      ],
       plugins: [`graphql`],
       rules: {
         // New versions of react use a special jsx runtime that remove the requirement
@@ -32,7 +92,7 @@ export const eslintConfig = (
             tagName: `graphql`,
           },
         ],
-        "react/jsx-pascal-case": `off`, // Prevents errors with Theme-UI and Styled component
+        "react/jsx-pascal-case": `warn`,
         // https://github.com/jsx-eslint/eslint-plugin-jsx-a11y/tree/master/docs/rules
         "jsx-a11y/accessible-emoji": `warn`,
         "jsx-a11y/alt-text": `warn`,
@@ -98,7 +158,7 @@ export const eslintConfig = (
             ],
           },
         ],
-        //"jsx-a11y/label-has-for": `warn`, was deprecated and replaced with jsx-a11y/has-associated-control in v6.1.0
+        // "jsx-a11y/label-has-for": `warn`, was deprecated and replaced with jsx-a11y/has-associated-control in v6.1.0
         "jsx-a11y/label-has-associated-control": `warn`,
         "jsx-a11y/lang": `warn`,
         "jsx-a11y/media-has-caption": `warn`,

packages/gatsby/src/utils/eslint-rules-helpers.ts

@@ -0,0 +1,22 @@
+import { Rule } from "eslint"
+import { GatsbyReduxStore } from "../redux"
+
+export function isPageTemplate(
+  s: GatsbyReduxStore,
+  c: Rule.RuleContext
+): boolean {
+  const filename = c.getFilename()
+  if (!filename) {
+    return false
+  }
+  return s.getState().components.has(filename)
+}
+
+export function test(t): any {
+  return Object.assign(t, {
+    parserOptions: {
+      sourceType: `module`,
+      ecmaVersion: 9,
+    },
+  })
+}