From 94449a6d6477d2b1cb3b371f9d48392586e8a784 Mon Sep 17 00:00:00 2001 From: Michal Piechowiak Date: Fri, 18 Dec 2020 13:17:53 +0100 Subject: [PATCH 01/23] tmp: add preliminary babel plugin for page templates to warn when those won't work with fast refresh --- .../gatsby/src/utils/babel-loader-helpers.js | 8 +++ ...ugin-page-template-support-fast-refresh.ts | 58 +++++++++++++++++++ 2 files changed, 66 insertions(+) create mode 100644 packages/gatsby/src/utils/babel-plugin-page-template-support-fast-refresh.ts diff --git a/packages/gatsby/src/utils/babel-loader-helpers.js b/packages/gatsby/src/utils/babel-loader-helpers.js index 081f5058f0829..18dec8334503a 100644 --- a/packages/gatsby/src/utils/babel-loader-helpers.js +++ b/packages/gatsby/src/utils/babel-loader-helpers.js @@ -56,6 +56,14 @@ const prepareOptions = (babel, options = {}, resolve = require.resolve) => { type: `plugin`, }) ) + requiredPlugins.push( + babel.createConfigItem( + [resolve(`./babel-plugin-page-template-support-fast-refresh`)], + { + type: `plugin`, + } + ) + ) } // TODO: Remove entire block when we make fast-refresh the default else { diff --git a/packages/gatsby/src/utils/babel-plugin-page-template-support-fast-refresh.ts b/packages/gatsby/src/utils/babel-plugin-page-template-support-fast-refresh.ts new file mode 100644 index 0000000000000..6621433705752 --- /dev/null +++ b/packages/gatsby/src/utils/babel-plugin-page-template-support-fast-refresh.ts @@ -0,0 +1,58 @@ +import { NodePath, PluginObj } from "@babel/core" +import { store } from "../redux" +import reporter from "gatsby-cli/lib/reporter" + +export default function ({ types: t, ...rest }): PluginObj { + return { + visitor: { + Program(path, state): void { + const filename = state.file.opts.filename + // TODO: verify on windows due to slashes hell + const isPageTemplate = store.getState().components.has(filename) + if (!isPageTemplate) { + return + } + + function makeWarning(path: NodePath, message: string) { + const codeFrame = path.buildCodeFrameError(message).message + reporter.warn(`${codeFrame}\n\nFilename: ${filename}`) + } + + path.traverse({ + ExportDefaultDeclaration: path => { + const declaration = path.node.declaration + if (declaration.type === `FunctionDeclaration`) { + if (!declaration.id) { + makeWarning(path, `I need a name`) + } else if ( + declaration.id?.name[0] !== + declaration.id?.name[0].toLocaleUpperCase() + ) { + makeWarning( + path, + `I need upper case. Change "${ + declaration.id.name + }" to "${declaration.id.name[0].toLocaleUpperCase()}${declaration.id.name.substring( + 1 + )}"` + ) + } + } else if (declaration.type === `ArrowFunctionExpression`) { + makeWarning( + path, + `Arrow function - do "const Blah = () => {}; export default Blah;". ` + ) + } + }, + ExportNamedDeclaration: path => { + // At this point page query export is already removed + makeWarning( + path, + `There are more exports than default (react template) and page query. Fast refresh won't work` + ) + }, + }) + }, + }, + } +} From ff80b61af8a709778919628db19be91ea1036bf4 Mon Sep 17 00:00:00 2001 From: Michal Piechowiak Date: Fri, 18 Dec 2020 13:25:26 +0100 Subject: [PATCH 02/23] slight reorder of warning message to be bit more readable (filename in middle, not in the end) --- .../babel-plugin-page-template-support-fast-refresh.ts | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/packages/gatsby/src/utils/babel-plugin-page-template-support-fast-refresh.ts b/packages/gatsby/src/utils/babel-plugin-page-template-support-fast-refresh.ts index 6621433705752..b4cf9d015a28e 100644 --- a/packages/gatsby/src/utils/babel-plugin-page-template-support-fast-refresh.ts +++ b/packages/gatsby/src/utils/babel-plugin-page-template-support-fast-refresh.ts @@ -14,8 +14,10 @@ export default function ({ types: t, ...rest }): PluginObj { } function makeWarning(path: NodePath, message: string) { - const codeFrame = path.buildCodeFrameError(message).message - reporter.warn(`${codeFrame}\n\nFilename: ${filename}`) + reporter.warn( + path.buildCodeFrameError(`${message}\n\nFilename: ${filename}\n\n`) + .message + ) } path.traverse({ From 82964c72ad94ccaf69adad905481ab1da8043e07 Mon Sep 17 00:00:00 2001 From: Michal Piechowiak Date: Fri, 18 Dec 2020 13:26:06 +0100 Subject: [PATCH 03/23] fix lint? --- .../utils/babel-plugin-page-template-support-fast-refresh.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/gatsby/src/utils/babel-plugin-page-template-support-fast-refresh.ts b/packages/gatsby/src/utils/babel-plugin-page-template-support-fast-refresh.ts index b4cf9d015a28e..204b8b064ad28 100644 --- a/packages/gatsby/src/utils/babel-plugin-page-template-support-fast-refresh.ts +++ b/packages/gatsby/src/utils/babel-plugin-page-template-support-fast-refresh.ts @@ -13,7 +13,7 @@ export default function ({ types: t, ...rest }): PluginObj { return } - function makeWarning(path: NodePath, message: string) { + function makeWarning(path: NodePath, message: string): void { reporter.warn( path.buildCodeFrameError(`${message}\n\nFilename: ${filename}\n\n`) .message From b12b7d6b3508b1bb76368335b13d0dc3ce4b2594 Mon Sep 17 00:00:00 2001 From: LekoArts Date: Wed, 6 Jan 2021 10:00:40 +0100 Subject: [PATCH 04/23] update messages --- ...bel-plugin-page-template-support-fast-refresh.ts | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/packages/gatsby/src/utils/babel-plugin-page-template-support-fast-refresh.ts b/packages/gatsby/src/utils/babel-plugin-page-template-support-fast-refresh.ts index 204b8b064ad28..c201f9fde5703 100644 --- a/packages/gatsby/src/utils/babel-plugin-page-template-support-fast-refresh.ts +++ b/packages/gatsby/src/utils/babel-plugin-page-template-support-fast-refresh.ts @@ -2,13 +2,20 @@ import { NodePath, PluginObj } from "@babel/core" import { store } from "../redux" import reporter from "gatsby-cli/lib/reporter" -export default function ({ types: t, ...rest }): PluginObj { +const anonymousMessage = `` + +export default function babelPluginPageTemplateSupportFastRefresh(): PluginObj { return { visitor: { Program(path, state): void { - const filename = state.file.opts.filename - // TODO: verify on windows due to slashes hell + const filename = state?.file?.opts?.filename + + if (!filename) { + return + } + const isPageTemplate = store.getState().components.has(filename) + if (!isPageTemplate) { return } From 2ea5561f7b434d1bb5a537ddeddeaca5f48379ce Mon Sep 17 00:00:00 2001 From: LekoArts Date: Wed, 6 Jan 2021 12:22:24 +0100 Subject: [PATCH 05/23] Improve warning messages and also add warning to other default exported lowercase components --- ...ugin-page-template-support-fast-refresh.ts | 56 +++++++++++++++++-- 1 file changed, 50 insertions(+), 6 deletions(-) diff --git a/packages/gatsby/src/utils/babel-plugin-page-template-support-fast-refresh.ts b/packages/gatsby/src/utils/babel-plugin-page-template-support-fast-refresh.ts index c201f9fde5703..e1a81106a039e 100644 --- a/packages/gatsby/src/utils/babel-plugin-page-template-support-fast-refresh.ts +++ b/packages/gatsby/src/utils/babel-plugin-page-template-support-fast-refresh.ts @@ -2,8 +2,6 @@ import { NodePath, PluginObj } from "@babel/core" import { store } from "../redux" import reporter from "gatsby-cli/lib/reporter" -const anonymousMessage = `` - export default function babelPluginPageTemplateSupportFastRefresh(): PluginObj { return { visitor: { @@ -32,14 +30,30 @@ export default function babelPluginPageTemplateSupportFastRefresh(): PluginObj { const declaration = path.node.declaration if (declaration.type === `FunctionDeclaration`) { if (!declaration.id) { - makeWarning(path, `I need a name`) + makeWarning( + path, + `[FAST REFRESH] +Anonymous function declarations cause Fast Refresh to not preserve local component state. + +Please add a name to your function, for example: + +Before: +export default function () {}; + +After: +export default function Named() {} +` + ) } else if ( declaration.id?.name[0] !== declaration.id?.name[0].toLocaleUpperCase() ) { makeWarning( path, - `I need upper case. Change "${ + `[FAST REFRESH] +Lowercase components cause Fast Refresh to not preserve local component state. You must use PascalCase. + +Change "${ declaration.id.name }" to "${declaration.id.name[0].toLocaleUpperCase()}${declaration.id.name.substring( 1 @@ -49,7 +63,33 @@ export default function babelPluginPageTemplateSupportFastRefresh(): PluginObj { } else if (declaration.type === `ArrowFunctionExpression`) { makeWarning( path, - `Arrow function - do "const Blah = () => {}; export default Blah;". ` + `[FAST REFRESH] +Anonymous arrow functions cause Fast Refresh to not preserve local component state. + +Please add a name to your function, for example: + +Before: +export default () => {}; + +After: +const Named = () => {}; +export default Named; +` + ) + } else if ( + declaration.type === `Identifier` && + declaration?.name[0] !== declaration?.name[0].toLocaleUpperCase() + ) { + makeWarning( + path, + `[FAST REFRESH] +Lowercase components cause Fast Refresh to not preserve local component state. You must use PascalCase. + +Change the component and default export from "${ + declaration.name + }" to "${declaration.name[0].toLocaleUpperCase()}${declaration.name.substring( + 1 + )}"` ) } }, @@ -57,7 +97,11 @@ export default function babelPluginPageTemplateSupportFastRefresh(): PluginObj { // At this point page query export is already removed makeWarning( path, - `There are more exports than default (react template) and page query. Fast refresh won't work` + `[FAST REFRESH] +In page templates only a default export of a valid React component and the named export of a page query is allowed. +All other named exports will cause Fast Refresh to not preserve local component state and do a full refresh. + +Please move your other named exports to another file.` ) }, }) From 69db5a372fedd872c37673c15543b652401a1a1c Mon Sep 17 00:00:00 2001 From: LekoArts Date: Wed, 6 Jan 2021 13:14:50 +0100 Subject: [PATCH 06/23] add onWarning --- packages/gatsby/src/utils/babel-loader.js | 14 ++++++++++++ ...ugin-page-template-support-fast-refresh.ts | 22 ++++++++++++++++++- packages/gatsby/src/utils/start-server.ts | 2 +- 3 files changed, 36 insertions(+), 2 deletions(-) diff --git a/packages/gatsby/src/utils/babel-loader.js b/packages/gatsby/src/utils/babel-loader.js index 8d2e371d71c9b..4c6a68c88050b 100644 --- a/packages/gatsby/src/utils/babel-loader.js +++ b/packages/gatsby/src/utils/babel-loader.js @@ -61,6 +61,20 @@ module.exports = babelLoader.custom(babel => { fallbackPresets, ] = prepareOptions(babel, customOptions) + if (process.env.GATSBY_HOT_LOADER === `fast-refresh`) { + const emitWarning = this.emitWarning.bind(this) + Object.defineProperty(options.caller, `onWarning`, { + enumerable: false, + writable: false, + value: (options.caller.onWarning = function (reason) { + if (!(reason instanceof Error)) { + reason = new Error(reason) + } + emitWarning(reason) + }), + }) + } + // If there is no filesystem babel config present, add our fallback // presets/plugins. if (!partialConfig.hasFilesystemConfig()) { diff --git a/packages/gatsby/src/utils/babel-plugin-page-template-support-fast-refresh.ts b/packages/gatsby/src/utils/babel-plugin-page-template-support-fast-refresh.ts index e1a81106a039e..5870026794735 100644 --- a/packages/gatsby/src/utils/babel-plugin-page-template-support-fast-refresh.ts +++ b/packages/gatsby/src/utils/babel-plugin-page-template-support-fast-refresh.ts @@ -2,7 +2,20 @@ import { NodePath, PluginObj } from "@babel/core" import { store } from "../redux" import reporter from "gatsby-cli/lib/reporter" -export default function babelPluginPageTemplateSupportFastRefresh(): PluginObj { +export default function babelPluginPageTemplateSupportFastRefresh({ + ...babel +}): PluginObj { + let onWarning: ((reason: string | Error) => void) | null = null + const hasOnWarning = typeof onWarning === `function` + babel.caller(caller => { + onWarning = caller.onWarning + return `` // Intentionally empty to not invalidate cache + }) + + console.log({ onWarning }) + + const warn = onWarning + return { visitor: { Program(path, state): void { @@ -23,6 +36,13 @@ export default function babelPluginPageTemplateSupportFastRefresh(): PluginObj { path.buildCodeFrameError(`${message}\n\nFilename: ${filename}\n\n`) .message ) + if (hasOnWarning) { + warn( + path.buildCodeFrameError( + `${message}\n\nFilename: ${filename}\n\n` + ).message + ) + } } path.traverse({ diff --git a/packages/gatsby/src/utils/start-server.ts b/packages/gatsby/src/utils/start-server.ts index dc11759ffd83c..710298b110af8 100644 --- a/packages/gatsby/src/utils/start-server.ts +++ b/packages/gatsby/src/utils/start-server.ts @@ -335,7 +335,7 @@ module.exports = { logLevel: `silent`, publicPath: devConfig.output.publicPath, watchOptions: devConfig.devServer ? devConfig.devServer.watchOptions : null, - stats: `errors-only`, + stats: `errors-warnings`, serverSideRender: true, }) as unknown) as PatchedWebpackDevMiddleware From 40a4ee19fed3ca2ade9fb43ed3a688c40d9b17e6 Mon Sep 17 00:00:00 2001 From: LekoArts Date: Wed, 6 Jan 2021 14:36:50 +0100 Subject: [PATCH 07/23] revert previous changes --- packages/gatsby/src/utils/babel-loader.js | 14 ------------ ...ugin-page-template-support-fast-refresh.ts | 22 +------------------ packages/gatsby/src/utils/start-server.ts | 2 +- 3 files changed, 2 insertions(+), 36 deletions(-) diff --git a/packages/gatsby/src/utils/babel-loader.js b/packages/gatsby/src/utils/babel-loader.js index 4c6a68c88050b..8d2e371d71c9b 100644 --- a/packages/gatsby/src/utils/babel-loader.js +++ b/packages/gatsby/src/utils/babel-loader.js @@ -61,20 +61,6 @@ module.exports = babelLoader.custom(babel => { fallbackPresets, ] = prepareOptions(babel, customOptions) - if (process.env.GATSBY_HOT_LOADER === `fast-refresh`) { - const emitWarning = this.emitWarning.bind(this) - Object.defineProperty(options.caller, `onWarning`, { - enumerable: false, - writable: false, - value: (options.caller.onWarning = function (reason) { - if (!(reason instanceof Error)) { - reason = new Error(reason) - } - emitWarning(reason) - }), - }) - } - // If there is no filesystem babel config present, add our fallback // presets/plugins. if (!partialConfig.hasFilesystemConfig()) { diff --git a/packages/gatsby/src/utils/babel-plugin-page-template-support-fast-refresh.ts b/packages/gatsby/src/utils/babel-plugin-page-template-support-fast-refresh.ts index 5870026794735..e1a81106a039e 100644 --- a/packages/gatsby/src/utils/babel-plugin-page-template-support-fast-refresh.ts +++ b/packages/gatsby/src/utils/babel-plugin-page-template-support-fast-refresh.ts @@ -2,20 +2,7 @@ import { NodePath, PluginObj } from "@babel/core" import { store } from "../redux" import reporter from "gatsby-cli/lib/reporter" -export default function babelPluginPageTemplateSupportFastRefresh({ - ...babel -}): PluginObj { - let onWarning: ((reason: string | Error) => void) | null = null - const hasOnWarning = typeof onWarning === `function` - babel.caller(caller => { - onWarning = caller.onWarning - return `` // Intentionally empty to not invalidate cache - }) - - console.log({ onWarning }) - - const warn = onWarning - +export default function babelPluginPageTemplateSupportFastRefresh(): PluginObj { return { visitor: { Program(path, state): void { @@ -36,13 +23,6 @@ export default function babelPluginPageTemplateSupportFastRefresh({ path.buildCodeFrameError(`${message}\n\nFilename: ${filename}\n\n`) .message ) - if (hasOnWarning) { - warn( - path.buildCodeFrameError( - `${message}\n\nFilename: ${filename}\n\n` - ).message - ) - } } path.traverse({ diff --git a/packages/gatsby/src/utils/start-server.ts b/packages/gatsby/src/utils/start-server.ts index 710298b110af8..dc11759ffd83c 100644 --- a/packages/gatsby/src/utils/start-server.ts +++ b/packages/gatsby/src/utils/start-server.ts @@ -335,7 +335,7 @@ module.exports = { logLevel: `silent`, publicPath: devConfig.output.publicPath, watchOptions: devConfig.devServer ? devConfig.devServer.watchOptions : null, - stats: `errors-warnings`, + stats: `errors-only`, serverSideRender: true, }) as unknown) as PatchedWebpackDevMiddleware From b120c37d49a852420b14283e392462dd2836495f Mon Sep 17 00:00:00 2001 From: LekoArts Date: Thu, 7 Jan 2021 13:07:45 +0100 Subject: [PATCH 08/23] revert previous changes --- .../gatsby/src/utils/babel-loader-helpers.js | 8 -- ...ugin-page-template-support-fast-refresh.ts | 111 ------------------ 2 files changed, 119 deletions(-) delete mode 100644 packages/gatsby/src/utils/babel-plugin-page-template-support-fast-refresh.ts diff --git a/packages/gatsby/src/utils/babel-loader-helpers.js b/packages/gatsby/src/utils/babel-loader-helpers.js index 18dec8334503a..081f5058f0829 100644 --- a/packages/gatsby/src/utils/babel-loader-helpers.js +++ b/packages/gatsby/src/utils/babel-loader-helpers.js @@ -56,14 +56,6 @@ const prepareOptions = (babel, options = {}, resolve = require.resolve) => { type: `plugin`, }) ) - requiredPlugins.push( - babel.createConfigItem( - [resolve(`./babel-plugin-page-template-support-fast-refresh`)], - { - type: `plugin`, - } - ) - ) } // TODO: Remove entire block when we make fast-refresh the default else { diff --git a/packages/gatsby/src/utils/babel-plugin-page-template-support-fast-refresh.ts b/packages/gatsby/src/utils/babel-plugin-page-template-support-fast-refresh.ts deleted file mode 100644 index e1a81106a039e..0000000000000 --- a/packages/gatsby/src/utils/babel-plugin-page-template-support-fast-refresh.ts +++ /dev/null @@ -1,111 +0,0 @@ -import { NodePath, PluginObj } from "@babel/core" -import { store } from "../redux" -import reporter from "gatsby-cli/lib/reporter" - -export default function babelPluginPageTemplateSupportFastRefresh(): PluginObj { - return { - visitor: { - Program(path, state): void { - const filename = state?.file?.opts?.filename - - if (!filename) { - return - } - - const isPageTemplate = store.getState().components.has(filename) - - if (!isPageTemplate) { - return - } - - function makeWarning(path: NodePath, message: string): void { - reporter.warn( - path.buildCodeFrameError(`${message}\n\nFilename: ${filename}\n\n`) - .message - ) - } - - path.traverse({ - ExportDefaultDeclaration: path => { - const declaration = path.node.declaration - if (declaration.type === `FunctionDeclaration`) { - if (!declaration.id) { - makeWarning( - path, - `[FAST REFRESH] -Anonymous function declarations cause Fast Refresh to not preserve local component state. - -Please add a name to your function, for example: - -Before: -export default function () {}; - -After: -export default function Named() {} -` - ) - } else if ( - declaration.id?.name[0] !== - declaration.id?.name[0].toLocaleUpperCase() - ) { - makeWarning( - path, - `[FAST REFRESH] -Lowercase components cause Fast Refresh to not preserve local component state. You must use PascalCase. - -Change "${ - declaration.id.name - }" to "${declaration.id.name[0].toLocaleUpperCase()}${declaration.id.name.substring( - 1 - )}"` - ) - } - } else if (declaration.type === `ArrowFunctionExpression`) { - makeWarning( - path, - `[FAST REFRESH] -Anonymous arrow functions cause Fast Refresh to not preserve local component state. - -Please add a name to your function, for example: - -Before: -export default () => {}; - -After: -const Named = () => {}; -export default Named; -` - ) - } else if ( - declaration.type === `Identifier` && - declaration?.name[0] !== declaration?.name[0].toLocaleUpperCase() - ) { - makeWarning( - path, - `[FAST REFRESH] -Lowercase components cause Fast Refresh to not preserve local component state. You must use PascalCase. - -Change the component and default export from "${ - declaration.name - }" to "${declaration.name[0].toLocaleUpperCase()}${declaration.name.substring( - 1 - )}"` - ) - } - }, - ExportNamedDeclaration: path => { - // At this point page query export is already removed - makeWarning( - path, - `[FAST REFRESH] -In page templates only a default export of a valid React component and the named export of a page query is allowed. -All other named exports will cause Fast Refresh to not preserve local component state and do a full refresh. - -Please move your other named exports to another file.` - ) - }, - }) - }, - }, - } -} From aeb80402e86641632485289823155daf6b6893d2 Mon Sep 17 00:00:00 2001 From: LekoArts Date: Thu, 7 Jan 2021 13:08:26 +0100 Subject: [PATCH 09/23] use two local rules --- packages/gatsby/src/utils/eslint-config.ts | 8 ++- .../gatsby/src/utils/eslint-rules-helpers.ts | 13 +++++ .../limited-exports-page-templates.ts | 28 ++++++++++ .../no-anonymous-exports-page-templates.ts | 55 +++++++++++++++++++ 4 files changed, 102 insertions(+), 2 deletions(-) create mode 100644 packages/gatsby/src/utils/eslint-rules-helpers.ts create mode 100644 packages/gatsby/src/utils/eslint-rules/limited-exports-page-templates.ts create mode 100644 packages/gatsby/src/utils/eslint-rules/no-anonymous-exports-page-templates.ts diff --git a/packages/gatsby/src/utils/eslint-config.ts b/packages/gatsby/src/utils/eslint-config.ts index e5b5c64ca1f9a..c4c6397041842 100644 --- a/packages/gatsby/src/utils/eslint-config.ts +++ b/packages/gatsby/src/utils/eslint-config.ts @@ -8,6 +8,7 @@ export const eslintConfig = ( return { useEslintrc: false, resolvePluginsRelativeTo: __dirname, + rulePaths: [`${__dirname}/eslint-rules`], baseConfig: { globals: { graphql: true, @@ -17,6 +18,9 @@ export const eslintConfig = ( extends: [require.resolve(`eslint-config-react-app`)], plugins: [`graphql`], rules: { + // Custom ESLint rules from Gatsby + "no-anonymous-exports-page-templates": `warn`, + "limited-exports-page-templates": `warn`, // New versions of react use a special jsx runtime that remove the requirement // for having react in scope for jsx. Once the jsx runtime is backported to all // versions of react we can make this always be `off`. @@ -32,7 +36,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 +102,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`, diff --git a/packages/gatsby/src/utils/eslint-rules-helpers.ts b/packages/gatsby/src/utils/eslint-rules-helpers.ts new file mode 100644 index 0000000000000..01641d98614ea --- /dev/null +++ b/packages/gatsby/src/utils/eslint-rules-helpers.ts @@ -0,0 +1,13 @@ +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) +} diff --git a/packages/gatsby/src/utils/eslint-rules/limited-exports-page-templates.ts b/packages/gatsby/src/utils/eslint-rules/limited-exports-page-templates.ts new file mode 100644 index 0000000000000..513c649d7d22c --- /dev/null +++ b/packages/gatsby/src/utils/eslint-rules/limited-exports-page-templates.ts @@ -0,0 +1,28 @@ +import { Rule } from "eslint" +import { store } from "../../redux" +import { isPageTemplate } from "../eslint-rules-helpers" + +const limitedExports: Rule.RuleModule = { + meta: { + type: `suggestion`, + }, + create: context => { + if (!isPageTemplate(store, context)) { + return undefined + } + + return { + ExportNamedDeclaration: (node): void => { + context.report({ + node, + message: `In page templates only a default export of a valid React component and the named export of a page query is allowed. +All other named exports will cause Fast Refresh to not preserve local component state and do a full refresh. + +Please move your other named exports to another file.`, + }) + }, + } + }, +} + +module.exports = limitedExports diff --git a/packages/gatsby/src/utils/eslint-rules/no-anonymous-exports-page-templates.ts b/packages/gatsby/src/utils/eslint-rules/no-anonymous-exports-page-templates.ts new file mode 100644 index 0000000000000..5fe979fa591b7 --- /dev/null +++ b/packages/gatsby/src/utils/eslint-rules/no-anonymous-exports-page-templates.ts @@ -0,0 +1,55 @@ +import { Rule } from "eslint" +import { store } from "../../redux" +import { isPageTemplate } from "../eslint-rules-helpers" + +const defs = { + ArrowFunctionExpression: { + message: `Anonymous arrow functions cause Fast Refresh to not preserve local component state. + +Please add a name to your function, for example: + +Before: +export default () => {}; + +After: +const Named = () => {}; +export default Named; +`, + }, + FunctionDeclaration: { + message: `Anonymous function declarations cause Fast Refresh to not preserve local component state. + +Please add a name to your function, for example: + +Before: +export default function () {}; + +After: +export default function Named() {} +`, + forbid: (node): boolean => !node.declaration.id, + }, +} + +const noAnonymousExports: Rule.RuleModule = { + meta: { + type: `suggestion`, + }, + create: context => { + if (!isPageTemplate(store, context)) { + return undefined + } + + return { + ExportDefaultDeclaration: (node): void => { + const def = defs[node.declaration.type] + + if (def && (!def.forbid || def.forbid(node))) { + context.report({ node, message: def.message }) + } + }, + } + }, +} + +module.exports = noAnonymousExports From a6b7c5e9d443531025197cb450a4b514eae83719 Mon Sep 17 00:00:00 2001 From: LekoArts Date: Thu, 7 Jan 2021 16:26:29 +0100 Subject: [PATCH 10/23] return correct empty object --- .../utils/eslint-rules/limited-exports-page-templates.ts | 2 +- .../eslint-rules/no-anonymous-exports-page-templates.ts | 9 ++++++--- 2 files changed, 7 insertions(+), 4 deletions(-) diff --git a/packages/gatsby/src/utils/eslint-rules/limited-exports-page-templates.ts b/packages/gatsby/src/utils/eslint-rules/limited-exports-page-templates.ts index 513c649d7d22c..074f32929477f 100644 --- a/packages/gatsby/src/utils/eslint-rules/limited-exports-page-templates.ts +++ b/packages/gatsby/src/utils/eslint-rules/limited-exports-page-templates.ts @@ -8,7 +8,7 @@ const limitedExports: Rule.RuleModule = { }, create: context => { if (!isPageTemplate(store, context)) { - return undefined + return {} } return { diff --git a/packages/gatsby/src/utils/eslint-rules/no-anonymous-exports-page-templates.ts b/packages/gatsby/src/utils/eslint-rules/no-anonymous-exports-page-templates.ts index 5fe979fa591b7..86b48e5bab70a 100644 --- a/packages/gatsby/src/utils/eslint-rules/no-anonymous-exports-page-templates.ts +++ b/packages/gatsby/src/utils/eslint-rules/no-anonymous-exports-page-templates.ts @@ -1,4 +1,5 @@ import { Rule } from "eslint" +import { Node, ExportDefaultDeclaration } from "estree" import { store } from "../../redux" import { isPageTemplate } from "../eslint-rules-helpers" @@ -37,12 +38,14 @@ const noAnonymousExports: Rule.RuleModule = { }, create: context => { if (!isPageTemplate(store, context)) { - return undefined + return {} } return { - ExportDefaultDeclaration: (node): void => { - const def = defs[node.declaration.type] + ExportDefaultDeclaration: (node: Node): void => { + // @ts-ignore + const type = node.declaration.type as ExportDefaultDeclaration["type"] + const def = defs[type] if (def && (!def.forbid || def.forbid(node))) { context.report({ node, message: def.message }) From 72c0e5a4e3c31e5d62e53da79e126c16c5cc1173 Mon Sep 17 00:00:00 2001 From: LekoArts Date: Thu, 7 Jan 2021 17:24:56 +0100 Subject: [PATCH 11/23] exlude template queries --- .../limited-exports-page-templates.ts | 19 ++++++++++++++++++- .../no-anonymous-exports-page-templates.ts | 2 +- 2 files changed, 19 insertions(+), 2 deletions(-) diff --git a/packages/gatsby/src/utils/eslint-rules/limited-exports-page-templates.ts b/packages/gatsby/src/utils/eslint-rules/limited-exports-page-templates.ts index 074f32929477f..cf7facaa686ee 100644 --- a/packages/gatsby/src/utils/eslint-rules/limited-exports-page-templates.ts +++ b/packages/gatsby/src/utils/eslint-rules/limited-exports-page-templates.ts @@ -1,10 +1,22 @@ import { Rule } from "eslint" +import { Node, Identifier } from "estree" import { store } from "../../redux" import { isPageTemplate } from "../eslint-rules-helpers" +function isTemplateQuery(node: Node): boolean { + return ( + node?.type === `ExportNamedDeclaration` && + node?.declaration?.type === `VariableDeclaration` && + node?.declaration?.declarations[0]?.init?.type === + `TaggedTemplateExpression` && + (node?.declaration?.declarations[0]?.init?.tag as Identifier)?.name === + `graphql` + ) +} + const limitedExports: Rule.RuleModule = { meta: { - type: `suggestion`, + type: `problem`, }, create: context => { if (!isPageTemplate(store, context)) { @@ -12,7 +24,12 @@ const limitedExports: Rule.RuleModule = { } return { + // eslint-disable-next-line consistent-return ExportNamedDeclaration: (node): void => { + if (isTemplateQuery(node)) { + return undefined + } + context.report({ node, message: `In page templates only a default export of a valid React component and the named export of a page query is allowed. diff --git a/packages/gatsby/src/utils/eslint-rules/no-anonymous-exports-page-templates.ts b/packages/gatsby/src/utils/eslint-rules/no-anonymous-exports-page-templates.ts index 86b48e5bab70a..aebbdd68f660a 100644 --- a/packages/gatsby/src/utils/eslint-rules/no-anonymous-exports-page-templates.ts +++ b/packages/gatsby/src/utils/eslint-rules/no-anonymous-exports-page-templates.ts @@ -34,7 +34,7 @@ export default function Named() {} const noAnonymousExports: Rule.RuleModule = { meta: { - type: `suggestion`, + type: `problem`, }, create: context => { if (!isPageTemplate(store, context)) { From fd895b2f6d56a4eee474284af6d7d022f326b3bc Mon Sep 17 00:00:00 2001 From: LekoArts Date: Fri, 8 Jan 2021 11:52:10 +0100 Subject: [PATCH 12/23] add e2e tests --- .../limited-exports-page-templates.js | 14 ++++++++++ .../no-anonymous-exports-page-templates.js | 22 +++++++++++++++ .../limited-exports-page-templates.js | 27 +++++++++++++++++++ ...onymous-exports-page-templates-function.js | 11 ++++++++ .../no-anonymous-exports-page-templates.js | 9 +++++++ 5 files changed, 83 insertions(+) create mode 100644 e2e-tests/development-runtime/cypress/integration/eslint-rules/limited-exports-page-templates.js create mode 100644 e2e-tests/development-runtime/cypress/integration/eslint-rules/no-anonymous-exports-page-templates.js create mode 100644 e2e-tests/development-runtime/src/pages/eslint-rules/limited-exports-page-templates.js create mode 100644 e2e-tests/development-runtime/src/pages/eslint-rules/no-anonymous-exports-page-templates-function.js create mode 100644 e2e-tests/development-runtime/src/pages/eslint-rules/no-anonymous-exports-page-templates.js diff --git a/e2e-tests/development-runtime/cypress/integration/eslint-rules/limited-exports-page-templates.js b/e2e-tests/development-runtime/cypress/integration/eslint-rules/limited-exports-page-templates.js new file mode 100644 index 0000000000000..3464d8de60a7a --- /dev/null +++ b/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) + }) + }) +} diff --git a/e2e-tests/development-runtime/cypress/integration/eslint-rules/no-anonymous-exports-page-templates.js b/e2e-tests/development-runtime/cypress/integration/eslint-rules/no-anonymous-exports-page-templates.js new file mode 100644 index 0000000000000..53aacb74fea8b --- /dev/null +++ b/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) + }) + }) +} diff --git a/e2e-tests/development-runtime/src/pages/eslint-rules/limited-exports-page-templates.js b/e2e-tests/development-runtime/src/pages/eslint-rules/limited-exports-page-templates.js new file mode 100644 index 0000000000000..75a40504bb908 --- /dev/null +++ b/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 ( +
+

Limited Exports Page Templates. ESLint Rule

+

+ {data.site.siteMetadata.title} +

+
+ ) +} + +export function notAllowed() {} + +export const query = graphql` + { + site { + siteMetadata { + title + } + } + } +` + +export default PageQuery diff --git a/e2e-tests/development-runtime/src/pages/eslint-rules/no-anonymous-exports-page-templates-function.js b/e2e-tests/development-runtime/src/pages/eslint-rules/no-anonymous-exports-page-templates-function.js new file mode 100644 index 0000000000000..e7ef2e9fbd5a1 --- /dev/null +++ b/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 ( + +

Anonymous Arrow Function. ESLint Rule

+
+ ) +} diff --git a/e2e-tests/development-runtime/src/pages/eslint-rules/no-anonymous-exports-page-templates.js b/e2e-tests/development-runtime/src/pages/eslint-rules/no-anonymous-exports-page-templates.js new file mode 100644 index 0000000000000..4378c7bbcb324 --- /dev/null +++ b/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 () => ( + +

Anonymous Arrow Function. ESLint Rule

+
+) From a5b8305cd6efe5b7dd3306f25c587373252c5d4c Mon Sep 17 00:00:00 2001 From: LekoArts Date: Fri, 8 Jan 2021 15:05:21 +0100 Subject: [PATCH 13/23] add first unit test --- .../gatsby/src/utils/eslint-rules-helpers.ts | 9 ++++ .../no-anonymous-exports-page-templates.ts | 47 +++++++++++++++++ .../limited-exports-page-templates.ts | 12 +++-- .../no-anonymous-exports-page-templates.ts | 51 +++++++++++-------- 4 files changed, 93 insertions(+), 26 deletions(-) create mode 100644 packages/gatsby/src/utils/eslint-rules/__tests__/no-anonymous-exports-page-templates.ts diff --git a/packages/gatsby/src/utils/eslint-rules-helpers.ts b/packages/gatsby/src/utils/eslint-rules-helpers.ts index 01641d98614ea..2cf1cd0f0fa3b 100644 --- a/packages/gatsby/src/utils/eslint-rules-helpers.ts +++ b/packages/gatsby/src/utils/eslint-rules-helpers.ts @@ -11,3 +11,12 @@ export function isPageTemplate( } return s.getState().components.has(filename) } + +export function test(t): any { + return Object.assign(t, { + parserOptions: { + sourceType: `module`, + ecmaVersion: 9, + }, + }) +} diff --git a/packages/gatsby/src/utils/eslint-rules/__tests__/no-anonymous-exports-page-templates.ts b/packages/gatsby/src/utils/eslint-rules/__tests__/no-anonymous-exports-page-templates.ts new file mode 100644 index 0000000000000..ec1cf85d39c81 --- /dev/null +++ b/packages/gatsby/src/utils/eslint-rules/__tests__/no-anonymous-exports-page-templates.ts @@ -0,0 +1,47 @@ +import { RuleTester } from "eslint" +import { test } from "../../eslint-rules-helpers" +const rule = require(`../no-anonymous-exports-page-templates`) + +const parserOptions = { + ecmaVersion: 2018, + sourceType: `module`, + ecmaFeatures: { + jsx: true, + }, +} + +const ruleTester = new RuleTester({ parserOptions }) + +jest.mock(`../../eslint-rules-helpers`, () => { + return { + ...jest.requireActual(`../../eslint-rules-helpers`), + isPageTemplate: jest.fn().mockReturnValue(true), + } +}) + +ruleTester.run(`no-anonymous-exports-page-templates`, rule, { + valid: [ + // Exports with identifiers are valid + test({ code: `const Named = () => {}\nexport default Named` }), + test({ code: `export default function foo() {}` }), + test({ code: `export default class MyClass {}` }), + + // Sanity check unrelated export syntaxes + test({ code: `export * from 'foo'` }), + test({ code: `const foo = 123\nexport { foo }` }), + test({ code: `const foo = 123\nexport { foo as default }` }), + + // Allow call expressions by default for backwards compatibility + test({ code: `export default foo(bar)` }), + ], + invalid: [ + test({ + code: `export default () => {}`, + errors: [{ messageId: `anonymousArrowFunction` }], + }), + test({ + code: `export default function() {}`, + errors: [{ messageId: `anonymousFunctionDeclaration` }], + }), + ], +}) diff --git a/packages/gatsby/src/utils/eslint-rules/limited-exports-page-templates.ts b/packages/gatsby/src/utils/eslint-rules/limited-exports-page-templates.ts index cf7facaa686ee..d692729add609 100644 --- a/packages/gatsby/src/utils/eslint-rules/limited-exports-page-templates.ts +++ b/packages/gatsby/src/utils/eslint-rules/limited-exports-page-templates.ts @@ -17,6 +17,13 @@ function isTemplateQuery(node: Node): boolean { const limitedExports: Rule.RuleModule = { meta: { type: `problem`, + messages: { + limitedExportsPageTemplates: `In page templates only a default export of a valid React component and the named export of a page query is allowed. + All other named exports will cause Fast Refresh to not preserve local component state and do a full refresh. + + Please move your other named exports to another file. +`, + }, }, create: context => { if (!isPageTemplate(store, context)) { @@ -32,10 +39,7 @@ const limitedExports: Rule.RuleModule = { context.report({ node, - message: `In page templates only a default export of a valid React component and the named export of a page query is allowed. -All other named exports will cause Fast Refresh to not preserve local component state and do a full refresh. - -Please move your other named exports to another file.`, + messageId: `limitedExportsPageTemplates`, }) }, } diff --git a/packages/gatsby/src/utils/eslint-rules/no-anonymous-exports-page-templates.ts b/packages/gatsby/src/utils/eslint-rules/no-anonymous-exports-page-templates.ts index aebbdd68f660a..7457bc93fd0fb 100644 --- a/packages/gatsby/src/utils/eslint-rules/no-anonymous-exports-page-templates.ts +++ b/packages/gatsby/src/utils/eslint-rules/no-anonymous-exports-page-templates.ts @@ -5,29 +5,10 @@ import { isPageTemplate } from "../eslint-rules-helpers" const defs = { ArrowFunctionExpression: { - message: `Anonymous arrow functions cause Fast Refresh to not preserve local component state. - -Please add a name to your function, for example: - -Before: -export default () => {}; - -After: -const Named = () => {}; -export default Named; -`, + messageId: `anonymousArrowFunction`, }, FunctionDeclaration: { - message: `Anonymous function declarations cause Fast Refresh to not preserve local component state. - -Please add a name to your function, for example: - -Before: -export default function () {}; - -After: -export default function Named() {} -`, + messageId: `anonymousFunctionDeclaration`, forbid: (node): boolean => !node.declaration.id, }, } @@ -35,6 +16,29 @@ export default function Named() {} const noAnonymousExports: Rule.RuleModule = { meta: { type: `problem`, + messages: { + anonymousArrowFunction: `Anonymous arrow functions cause Fast Refresh to not preserve local component state. + + Please add a name to your function, for example: + + Before: + export default () => {}; + + After: + const Named = () => {}; + export default Named; +`, + anonymousFunctionDeclaration: `Anonymous function declarations cause Fast Refresh to not preserve local component state. + + Please add a name to your function, for example: + + Before: + export default function () {}; + + After: + export default function Named() {} +`, + }, }, create: context => { if (!isPageTemplate(store, context)) { @@ -48,7 +52,10 @@ const noAnonymousExports: Rule.RuleModule = { const def = defs[type] if (def && (!def.forbid || def.forbid(node))) { - context.report({ node, message: def.message }) + context.report({ + node, + messageId: def.messageId, + }) } }, } From a356876f2b9fb1bb65b8ca10cdab8c3cbc0c7e9a Mon Sep 17 00:00:00 2001 From: LekoArts Date: Fri, 8 Jan 2021 16:50:23 +0100 Subject: [PATCH 14/23] unit test for page exports --- .../limited-exports-page-templates.ts | 44 +++++++++++++++++ .../limited-exports-page-templates.ts | 49 +++++++++++++++---- 2 files changed, 84 insertions(+), 9 deletions(-) create mode 100644 packages/gatsby/src/utils/eslint-rules/__tests__/limited-exports-page-templates.ts diff --git a/packages/gatsby/src/utils/eslint-rules/__tests__/limited-exports-page-templates.ts b/packages/gatsby/src/utils/eslint-rules/__tests__/limited-exports-page-templates.ts new file mode 100644 index 0000000000000..2fc8e378e9e75 --- /dev/null +++ b/packages/gatsby/src/utils/eslint-rules/__tests__/limited-exports-page-templates.ts @@ -0,0 +1,44 @@ +import { RuleTester } from "eslint" +import { test } from "../../eslint-rules-helpers" +const rule = require(`../limited-exports-page-templates`) + +const parserOptions = { + ecmaVersion: 2018, + sourceType: `module`, + ecmaFeatures: { + jsx: true, + }, +} + +const ruleTester = new RuleTester({ parserOptions }) + +jest.mock(`../../eslint-rules-helpers`, () => { + return { + ...jest.requireActual(`../../eslint-rules-helpers`), + isPageTemplate: jest.fn().mockReturnValue(true), + } +}) + +ruleTester.run(`no-anonymous-exports-page-templates`, rule, { + valid: [ + test({ + code: `const Template = () => {}\nexport const query = graphql\`test\`\nexport default Template`, + }), + test({ + code: `const Template = () => {}\nexport default Template`, + }), + test({ + code: `const Template = () => {}\nconst query = graphql\`test\`\nexport { query }\nexport default Template`, + }), + ], + invalid: [ + test({ + code: `const Template = () => {}\nexport const query = graphql\`test\`\nexport function Test() {}\nexport default Template`, + errors: [{ messageId: `limitedExportsPageTemplates` }], + }), + test({ + code: `const Template = () => {}\nconst query = graphql\`test\`\nfunction Test() {}\nexport { query, Test }\nexport default Template`, + errors: [{ messageId: `limitedExportsPageTemplates` }], + }), + ], +}) diff --git a/packages/gatsby/src/utils/eslint-rules/limited-exports-page-templates.ts b/packages/gatsby/src/utils/eslint-rules/limited-exports-page-templates.ts index d692729add609..dc1e50e8643d9 100644 --- a/packages/gatsby/src/utils/eslint-rules/limited-exports-page-templates.ts +++ b/packages/gatsby/src/utils/eslint-rules/limited-exports-page-templates.ts @@ -3,15 +3,34 @@ import { Node, Identifier } from "estree" import { store } from "../../redux" import { isPageTemplate } from "../eslint-rules-helpers" -function isTemplateQuery(node: Node): boolean { - return ( +function isTemplateQuery(node: Node, varName: string | undefined): boolean { + // Checks for: + // const query = graphql`` + // export { query } + if ( node?.type === `ExportNamedDeclaration` && - node?.declaration?.type === `VariableDeclaration` && - node?.declaration?.declarations[0]?.init?.type === - `TaggedTemplateExpression` && - (node?.declaration?.declarations[0]?.init?.tag as Identifier)?.name === - `graphql` - ) + node?.declaration === null && + varName + ) { + // For export { foobar } the declaration will be null and specifiers exists + const nonQueryExports = node.specifiers.find( + e => e.exported.name !== varName + ) + return !nonQueryExports + } else { + // For export const query = 'foobar' the declaration exists with type 'VariableDeclaration' + + // Checks for: + // export const query = graphql`` + return ( + node?.type === `ExportNamedDeclaration` && + node?.declaration?.type === `VariableDeclaration` && + node?.declaration?.declarations[0]?.init?.type === + `TaggedTemplateExpression` && + (node?.declaration?.declarations[0]?.init?.tag as Identifier)?.name === + `graphql` + ) + } } const limitedExports: Rule.RuleModule = { @@ -30,10 +49,22 @@ const limitedExports: Rule.RuleModule = { return {} } + let queryVariableName: string | undefined = `` + return { + TaggedTemplateExpression: (node): void => { + if ( + node?.type === `TaggedTemplateExpression` && + // @ts-ignore + node?.tag?.name === `graphql` + ) { + // @ts-ignore + queryVariableName = node?.parent?.id?.name + } + }, // eslint-disable-next-line consistent-return ExportNamedDeclaration: (node): void => { - if (isTemplateQuery(node)) { + if (isTemplateQuery(node, queryVariableName)) { return undefined } From 4ba90f7593bff09aab83ffec764f9e1c8742e6a0 Mon Sep 17 00:00:00 2001 From: LekoArts Date: Mon, 11 Jan 2021 15:58:43 +0100 Subject: [PATCH 15/23] review comments --- .../no-anonymous-exports-page-templates.ts | 4 ++++ .../limited-exports-page-templates.ts | 22 +++++++++++-------- .../no-anonymous-exports-page-templates.ts | 20 ++++++++++++++--- 3 files changed, 34 insertions(+), 12 deletions(-) diff --git a/packages/gatsby/src/utils/eslint-rules/__tests__/no-anonymous-exports-page-templates.ts b/packages/gatsby/src/utils/eslint-rules/__tests__/no-anonymous-exports-page-templates.ts index ec1cf85d39c81..f9095945767c2 100644 --- a/packages/gatsby/src/utils/eslint-rules/__tests__/no-anonymous-exports-page-templates.ts +++ b/packages/gatsby/src/utils/eslint-rules/__tests__/no-anonymous-exports-page-templates.ts @@ -43,5 +43,9 @@ ruleTester.run(`no-anonymous-exports-page-templates`, rule, { code: `export default function() {}`, errors: [{ messageId: `anonymousFunctionDeclaration` }], }), + test({ + code: `export default class {}`, + errors: [{ messageId: `anonymousClass` }], + }), ], }) diff --git a/packages/gatsby/src/utils/eslint-rules/limited-exports-page-templates.ts b/packages/gatsby/src/utils/eslint-rules/limited-exports-page-templates.ts index dc1e50e8643d9..06e041079f37b 100644 --- a/packages/gatsby/src/utils/eslint-rules/limited-exports-page-templates.ts +++ b/packages/gatsby/src/utils/eslint-rules/limited-exports-page-templates.ts @@ -8,8 +8,8 @@ function isTemplateQuery(node: Node, varName: string | undefined): boolean { // const query = graphql`` // export { query } if ( - node?.type === `ExportNamedDeclaration` && - node?.declaration === null && + node.type === `ExportNamedDeclaration` && + node.declaration === null && varName ) { // For export { foobar } the declaration will be null and specifiers exists @@ -23,11 +23,11 @@ function isTemplateQuery(node: Node, varName: string | undefined): boolean { // Checks for: // export const query = graphql`` return ( - node?.type === `ExportNamedDeclaration` && - node?.declaration?.type === `VariableDeclaration` && - node?.declaration?.declarations[0]?.init?.type === + node.type === `ExportNamedDeclaration` && + node.declaration?.type === `VariableDeclaration` && + node.declaration?.declarations[0]?.init?.type === `TaggedTemplateExpression` && - (node?.declaration?.declarations[0]?.init?.tag as Identifier)?.name === + (node.declaration?.declarations[0]?.init?.tag as Identifier)?.name === `graphql` ) } @@ -52,14 +52,18 @@ const limitedExports: Rule.RuleModule = { let queryVariableName: string | undefined = `` return { + // eslint-disable-next-line consistent-return TaggedTemplateExpression: (node): void => { if ( - node?.type === `TaggedTemplateExpression` && + node.type === `TaggedTemplateExpression` && // @ts-ignore - node?.tag?.name === `graphql` + node.tag?.name === `graphql` ) { + if (queryVariableName) { + return undefined + } // @ts-ignore - queryVariableName = node?.parent?.id?.name + queryVariableName = node.parent?.id?.name } }, // eslint-disable-next-line consistent-return diff --git a/packages/gatsby/src/utils/eslint-rules/no-anonymous-exports-page-templates.ts b/packages/gatsby/src/utils/eslint-rules/no-anonymous-exports-page-templates.ts index 7457bc93fd0fb..8b130aed86abc 100644 --- a/packages/gatsby/src/utils/eslint-rules/no-anonymous-exports-page-templates.ts +++ b/packages/gatsby/src/utils/eslint-rules/no-anonymous-exports-page-templates.ts @@ -11,6 +11,10 @@ const defs = { messageId: `anonymousFunctionDeclaration`, forbid: (node): boolean => !node.declaration.id, }, + ClassDeclaration: { + messageId: `anonymousClass`, + forbid: (node): boolean => !node.declaration.id, + }, } const noAnonymousExports: Rule.RuleModule = { @@ -22,10 +26,10 @@ const noAnonymousExports: Rule.RuleModule = { Please add a name to your function, for example: Before: - export default () => {}; + export default () => {} After: - const Named = () => {}; + const Named = () => {} export default Named; `, anonymousFunctionDeclaration: `Anonymous function declarations cause Fast Refresh to not preserve local component state. @@ -33,10 +37,20 @@ const noAnonymousExports: Rule.RuleModule = { Please add a name to your function, for example: Before: - export default function () {}; + export default function () {} After: export default function Named() {} +`, + anonymousClass: `Anonymous classes cause Fast Refresh to not preserve local component state. + + Please add a name to your class, for example: + + Before: + export default class extends Component {} + + After: + export default class Named extends Component {} `, }, }, From 908077ce67b2ba23341903b4f652be289c4a00a0 Mon Sep 17 00:00:00 2001 From: LekoArts Date: Tue, 12 Jan 2021 10:29:27 +0100 Subject: [PATCH 16/23] handle multiple declarations --- .../limited-exports-page-templates.ts | 8 +++ .../limited-exports-page-templates.ts | 50 +++++++++++++------ 2 files changed, 42 insertions(+), 16 deletions(-) diff --git a/packages/gatsby/src/utils/eslint-rules/__tests__/limited-exports-page-templates.ts b/packages/gatsby/src/utils/eslint-rules/__tests__/limited-exports-page-templates.ts index 2fc8e378e9e75..03ae3afce518c 100644 --- a/packages/gatsby/src/utils/eslint-rules/__tests__/limited-exports-page-templates.ts +++ b/packages/gatsby/src/utils/eslint-rules/__tests__/limited-exports-page-templates.ts @@ -40,5 +40,13 @@ ruleTester.run(`no-anonymous-exports-page-templates`, rule, { code: `const Template = () => {}\nconst query = graphql\`test\`\nfunction Test() {}\nexport { query, Test }\nexport default Template`, errors: [{ messageId: `limitedExportsPageTemplates` }], }), + test({ + code: `const Template = () => {}\nexport const query = graphql\`test\`, hello = 10\nexport default Template`, + errors: [{ messageId: `limitedExportsPageTemplates` }], + }), + test({ + code: `const Template = () => {}\nexport const hello = 10, query = graphql\`test\`\nexport default Template`, + errors: [{ messageId: `limitedExportsPageTemplates` }], + }), ], }) diff --git a/packages/gatsby/src/utils/eslint-rules/limited-exports-page-templates.ts b/packages/gatsby/src/utils/eslint-rules/limited-exports-page-templates.ts index 06e041079f37b..167b775882319 100644 --- a/packages/gatsby/src/utils/eslint-rules/limited-exports-page-templates.ts +++ b/packages/gatsby/src/utils/eslint-rules/limited-exports-page-templates.ts @@ -3,7 +3,10 @@ import { Node, Identifier } from "estree" import { store } from "../../redux" import { isPageTemplate } from "../eslint-rules-helpers" -function isTemplateQuery(node: Node, varName: string | undefined): boolean { +function hasOneValidNamedDeclaration( + node: Node, + varName: string | undefined +): boolean { // Checks for: // const query = graphql`` // export { query } @@ -12,25 +15,36 @@ function isTemplateQuery(node: Node, varName: string | undefined): boolean { node.declaration === null && varName ) { - // For export { foobar } the declaration will be null and specifiers exists - const nonQueryExports = node.specifiers.find( + // For export { foobar, query } the declaration will be null and specifiers exists + // For { foobar, query } it'll return true, for { query } it'll return false + const nonQueryExports = node.specifiers.some( e => e.exported.name !== varName ) return !nonQueryExports - } else { - // For export const query = 'foobar' the declaration exists with type 'VariableDeclaration' + } + + return false +} + +function isTemplateQuery(node: Node): boolean { + // For export const query = 'foobar' the declaration exists with type 'VariableDeclaration' + + // Checks for: + // export const query = graphql`` + // This case only has one item in the declarations array + // For export const hello = 10, world = 'foo' + // The array will have two items. So use every() to check if only one item exists + // With TaggedTemplateExpression and "graphql" name - // Checks for: - // export const query = graphql`` - return ( - node.type === `ExportNamedDeclaration` && - node.declaration?.type === `VariableDeclaration` && - node.declaration?.declarations[0]?.init?.type === - `TaggedTemplateExpression` && - (node.declaration?.declarations[0]?.init?.tag as Identifier)?.name === - `graphql` + return ( + node.type === `ExportNamedDeclaration` && + node.declaration?.type === `VariableDeclaration` && + node.declaration?.declarations.every( + el => + el?.init?.type === `TaggedTemplateExpression` && + (el.init?.tag as Identifier)?.name === `graphql` ) - } + ) } const limitedExports: Rule.RuleModule = { @@ -68,7 +82,11 @@ const limitedExports: Rule.RuleModule = { }, // eslint-disable-next-line consistent-return ExportNamedDeclaration: (node): void => { - if (isTemplateQuery(node, queryVariableName)) { + if (hasOneValidNamedDeclaration(node, queryVariableName)) { + return undefined + } + + if (isTemplateQuery(node)) { return undefined } From 24468c1b99c86fdde16857d39c1f6e96571c31d2 Mon Sep 17 00:00:00 2001 From: Michal Piechowiak Date: Wed, 13 Jan 2021 17:32:19 +0100 Subject: [PATCH 17/23] feat(gatsby): add required eslint rules even if user has custom eslint config (#28911) * feat(gatsby): add required eslint rules even if user has custom eslint config * handle case when extends is single string, add some tests * some extra existance checking * more exact checks for existance of eslint-loader rule * let's see if slash fixes win32 + env var for rules * another try * don't use slash after all Co-authored-by: LekoArts --- .../src/utils/__tests__/eslint-config.ts | 130 ++++++++++++++++++ packages/gatsby/src/utils/eslint-config.ts | 66 ++++++++- packages/gatsby/src/utils/eslint/required.js | 9 ++ packages/gatsby/src/utils/webpack-utils.ts | 58 +++++++- packages/gatsby/src/utils/webpack.config.js | 14 +- 5 files changed, 267 insertions(+), 10 deletions(-) create mode 100644 packages/gatsby/src/utils/__tests__/eslint-config.ts create mode 100644 packages/gatsby/src/utils/eslint/required.js diff --git a/packages/gatsby/src/utils/__tests__/eslint-config.ts b/packages/gatsby/src/utils/__tests__/eslint-config.ts new file mode 100644 index 0000000000000..e9fdf61f41822 --- /dev/null +++ b/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 [ + "/packages/gatsby/src/utils/eslint/required.js", + ], + } + `) + + expect(conf.rulePaths).toMatchInlineSnapshot(` + Array [ + "/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", + "/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 [ + "/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", + "/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", + "/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 [ + "/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": "/packages/gatsby/src/utils/eslint/required.js", + } + `) + }) + }) +}) diff --git a/packages/gatsby/src/utils/eslint-config.ts b/packages/gatsby/src/utils/eslint-config.ts index c4c6397041842..0064ab2dd07f3 100644 --- a/packages/gatsby/src/utils/eslint-config.ts +++ b/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,19 +64,19 @@ export const eslintConfig = ( return { useEslintrc: false, resolvePluginsRelativeTo: __dirname, - rulePaths: [`${__dirname}/eslint-rules`], + 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: { - // Custom ESLint rules from Gatsby - "no-anonymous-exports-page-templates": `warn`, - "limited-exports-page-templates": `warn`, // New versions of react use a special jsx runtime that remove the requirement // for having react in scope for jsx. Once the jsx runtime is backported to all // versions of react we can make this always be `off`. diff --git a/packages/gatsby/src/utils/eslint/required.js b/packages/gatsby/src/utils/eslint/required.js new file mode 100644 index 0000000000000..7ac22d57db856 --- /dev/null +++ b/packages/gatsby/src/utils/eslint/required.js @@ -0,0 +1,9 @@ +module.exports = { + rules: { + // Custom ESLint rules from Gatsby + "no-anonymous-exports-page-templates": + process.env.GATSBY_HOT_LOADER === `fast-refresh` ? `warn` : `off`, + "limited-exports-page-templates": + process.env.GATSBY_HOT_LOADER === `fast-refresh` ? `warn` : `off`, + }, +} diff --git a/packages/gatsby/src/utils/webpack-utils.ts b/packages/gatsby/src/utils/webpack-utils.ts index 77c942504f122..617a948c2000e 100644 --- a/packages/gatsby/src/utils/webpack-utils.ts +++ b/packages/gatsby/src/utils/webpack-utils.ts @@ -1,5 +1,5 @@ import * as path from "path" -import { Loader, RuleSetRule, Plugin } from "webpack" +import { Loader, RuleSetRule, Plugin, Configuration } from "webpack" import { GraphQLSchema } from "graphql" import postcss from "postcss" import autoprefixer from "autoprefixer" @@ -20,7 +20,11 @@ import { import { builtinPlugins } from "./webpack-plugins" import { IProgram, Stage } from "../commands/types" -import { eslintConfig } from "./eslint-config" +import { + eslintConfig, + mergeRequiredConfigIn, + eslintRequiredConfig, +} from "./eslint-config" type LoaderResolver = (options?: T) => Loader @@ -124,6 +128,8 @@ interface IWebpackUtils { plugins: PluginUtils } +const vendorRegex = /(node_modules|bower_components)/ + /** * A factory method that produces an atoms namespace */ @@ -132,7 +138,6 @@ export const createWebpackUtils = ( program: IProgram ): IWebpackUtils => { const assetRelativeRoot = `static/` - const vendorRegex = /(node_modules|bower_components)/ const supportedBrowsers = getBrowsersList(program.directory) const PRODUCTION = !stage.includes(`develop`) @@ -749,3 +754,50 @@ export function reactHasJsxRuntime(): boolean { return false } + +export function ensureRequireEslintRules(config: Configuration): Configuration { + if (!config.module) { + config.module = { + rules: [], + } + } + // for fast refresh we want to ensure that that there is eslint rule running + // because user might have added their own `eslint-loader` let's check if there is one + // and adjust it to add the rule or append new loader with required rule + const rule = config.module?.rules.find(rule => { + if (typeof rule.loader === `string`) { + return ( + rule.loader === `eslint-loader` || + rule.loader.endsWith(`eslint-loader/index.js`) || + rule.loader.endsWith(`eslint-loader/dist/cjs.js`) + ) + } + + return false + }) + + if (rule) { + if (typeof rule.options !== `string`) { + if (!rule.options) { + rule.options = {} + } + mergeRequiredConfigIn(rule.options) + } + } else { + config.module.rules.push({ + enforce: `pre`, + test: /\.jsx?$/, + exclude: (modulePath: string): boolean => + modulePath.includes(VIRTUAL_MODULES_BASE_PATH) || + vendorRegex.test(modulePath), + use: [ + { + loader: require.resolve(`eslint-loader`), + options: eslintRequiredConfig, + }, + ], + }) + } + + return config +} diff --git a/packages/gatsby/src/utils/webpack.config.js b/packages/gatsby/src/utils/webpack.config.js index 669d941995a28..9db7273c17770 100644 --- a/packages/gatsby/src/utils/webpack.config.js +++ b/packages/gatsby/src/utils/webpack.config.js @@ -15,7 +15,7 @@ const report = require(`gatsby-cli/lib/reporter`) import { withBasePath, withTrailingSlash } from "./path" import { getGatsbyDependents } from "./gatsby-dependents" const apiRunnerNode = require(`./api-runner-node`) -import { createWebpackUtils } from "./webpack-utils" +import { createWebpackUtils, ensureRequireEslintRules } from "./webpack-utils" import { hasLocalEslint } from "./local-eslint-config-finder" import { getAbsolutePathForVirtualModule } from "./gatsby-webpack-virtual-modules" @@ -732,5 +732,15 @@ module.exports = async ( parentSpan, }) - return getConfig() + let finalConfig = getConfig() + + if ( + stage === `develop` && + process.env.GATSBY_HOT_LOADER === `fast-refresh` && + hasLocalEslint(program.directory) + ) { + finalConfig = ensureRequireEslintRules(finalConfig) + } + + return finalConfig } From 8cf747599472213c3f27996eba9ace892e2de56d Mon Sep 17 00:00:00 2001 From: Lennart Date: Thu, 14 Jan 2021 11:22:05 +0100 Subject: [PATCH 18/23] Apply suggestions from code review Co-authored-by: Peter van der Zee <209817+pvdz@users.noreply.github.com> --- .../src/utils/eslint-rules/limited-exports-page-templates.ts | 2 +- packages/gatsby/src/utils/webpack-utils.ts | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/gatsby/src/utils/eslint-rules/limited-exports-page-templates.ts b/packages/gatsby/src/utils/eslint-rules/limited-exports-page-templates.ts index 167b775882319..3c48226398ba4 100644 --- a/packages/gatsby/src/utils/eslint-rules/limited-exports-page-templates.ts +++ b/packages/gatsby/src/utils/eslint-rules/limited-exports-page-templates.ts @@ -42,7 +42,7 @@ function isTemplateQuery(node: Node): boolean { node.declaration?.declarations.every( el => el?.init?.type === `TaggedTemplateExpression` && - (el.init?.tag as Identifier)?.name === `graphql` + (el.init.tag as Identifier)?.name === `graphql` ) ) } diff --git a/packages/gatsby/src/utils/webpack-utils.ts b/packages/gatsby/src/utils/webpack-utils.ts index 617a948c2000e..42255099e15b0 100644 --- a/packages/gatsby/src/utils/webpack-utils.ts +++ b/packages/gatsby/src/utils/webpack-utils.ts @@ -764,7 +764,7 @@ export function ensureRequireEslintRules(config: Configuration): Configuration { // for fast refresh we want to ensure that that there is eslint rule running // because user might have added their own `eslint-loader` let's check if there is one // and adjust it to add the rule or append new loader with required rule - const rule = config.module?.rules.find(rule => { + const rule = config.module.rules.find(rule => { if (typeof rule.loader === `string`) { return ( rule.loader === `eslint-loader` || From df460b24415b0cb6947f38e71c62a3b0476cdf53 Mon Sep 17 00:00:00 2001 From: LekoArts Date: Thu, 14 Jan 2021 15:09:49 +0100 Subject: [PATCH 19/23] fix consistent return --- .../utils/eslint-rules/limited-exports-page-templates.ts | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/packages/gatsby/src/utils/eslint-rules/limited-exports-page-templates.ts b/packages/gatsby/src/utils/eslint-rules/limited-exports-page-templates.ts index 3c48226398ba4..29039cf9d4e50 100644 --- a/packages/gatsby/src/utils/eslint-rules/limited-exports-page-templates.ts +++ b/packages/gatsby/src/utils/eslint-rules/limited-exports-page-templates.ts @@ -66,7 +66,6 @@ const limitedExports: Rule.RuleModule = { let queryVariableName: string | undefined = `` return { - // eslint-disable-next-line consistent-return TaggedTemplateExpression: (node): void => { if ( node.type === `TaggedTemplateExpression` && @@ -79,8 +78,9 @@ const limitedExports: Rule.RuleModule = { // @ts-ignore queryVariableName = node.parent?.id?.name } + + return undefined }, - // eslint-disable-next-line consistent-return ExportNamedDeclaration: (node): void => { if (hasOneValidNamedDeclaration(node, queryVariableName)) { return undefined @@ -94,6 +94,8 @@ const limitedExports: Rule.RuleModule = { node, messageId: `limitedExportsPageTemplates`, }) + + return undefined }, } }, From 2a814b93b7d5976eb658a6903b739321c7c2ee73 Mon Sep 17 00:00:00 2001 From: LekoArts Date: Fri, 15 Jan 2021 10:15:31 +0100 Subject: [PATCH 20/23] add describe block so that IDEs can run them :D --- .../limited-exports-page-templates.ts | 62 ++++++++++--------- .../no-anonymous-exports-page-templates.ts | 56 +++++++++-------- 2 files changed, 61 insertions(+), 57 deletions(-) diff --git a/packages/gatsby/src/utils/eslint-rules/__tests__/limited-exports-page-templates.ts b/packages/gatsby/src/utils/eslint-rules/__tests__/limited-exports-page-templates.ts index 03ae3afce518c..d2a28cf77cf67 100644 --- a/packages/gatsby/src/utils/eslint-rules/__tests__/limited-exports-page-templates.ts +++ b/packages/gatsby/src/utils/eslint-rules/__tests__/limited-exports-page-templates.ts @@ -19,34 +19,36 @@ jest.mock(`../../eslint-rules-helpers`, () => { } }) -ruleTester.run(`no-anonymous-exports-page-templates`, rule, { - valid: [ - test({ - code: `const Template = () => {}\nexport const query = graphql\`test\`\nexport default Template`, - }), - test({ - code: `const Template = () => {}\nexport default Template`, - }), - test({ - code: `const Template = () => {}\nconst query = graphql\`test\`\nexport { query }\nexport default Template`, - }), - ], - invalid: [ - test({ - code: `const Template = () => {}\nexport const query = graphql\`test\`\nexport function Test() {}\nexport default Template`, - errors: [{ messageId: `limitedExportsPageTemplates` }], - }), - test({ - code: `const Template = () => {}\nconst query = graphql\`test\`\nfunction Test() {}\nexport { query, Test }\nexport default Template`, - errors: [{ messageId: `limitedExportsPageTemplates` }], - }), - test({ - code: `const Template = () => {}\nexport const query = graphql\`test\`, hello = 10\nexport default Template`, - errors: [{ messageId: `limitedExportsPageTemplates` }], - }), - test({ - code: `const Template = () => {}\nexport const hello = 10, query = graphql\`test\`\nexport default Template`, - errors: [{ messageId: `limitedExportsPageTemplates` }], - }), - ], +describe(`no-anonymous-exports-page-templates`, () => { + ruleTester.run(`passes valid and invalid cases`, rule, { + valid: [ + test({ + code: `const Template = () => {}\nexport const query = graphql\`test\`\nexport default Template`, + }), + test({ + code: `const Template = () => {}\nexport default Template`, + }), + test({ + code: `const Template = () => {}\nconst query = graphql\`test\`\nexport { query }\nexport default Template`, + }), + ], + invalid: [ + test({ + code: `const Template = () => {}\nexport const query = graphql\`test\`\nexport function Test() {}\nexport default Template`, + errors: [{ messageId: `limitedExportsPageTemplates` }], + }), + test({ + code: `const Template = () => {}\nconst query = graphql\`test\`\nfunction Test() {}\nexport { query, Test }\nexport default Template`, + errors: [{ messageId: `limitedExportsPageTemplates` }], + }), + test({ + code: `const Template = () => {}\nexport const query = graphql\`test\`, hello = 10\nexport default Template`, + errors: [{ messageId: `limitedExportsPageTemplates` }], + }), + test({ + code: `const Template = () => {}\nexport const hello = 10, query = graphql\`test\`\nexport default Template`, + errors: [{ messageId: `limitedExportsPageTemplates` }], + }), + ], + }) }) diff --git a/packages/gatsby/src/utils/eslint-rules/__tests__/no-anonymous-exports-page-templates.ts b/packages/gatsby/src/utils/eslint-rules/__tests__/no-anonymous-exports-page-templates.ts index f9095945767c2..959790489ae53 100644 --- a/packages/gatsby/src/utils/eslint-rules/__tests__/no-anonymous-exports-page-templates.ts +++ b/packages/gatsby/src/utils/eslint-rules/__tests__/no-anonymous-exports-page-templates.ts @@ -19,33 +19,35 @@ jest.mock(`../../eslint-rules-helpers`, () => { } }) -ruleTester.run(`no-anonymous-exports-page-templates`, rule, { - valid: [ - // Exports with identifiers are valid - test({ code: `const Named = () => {}\nexport default Named` }), - test({ code: `export default function foo() {}` }), - test({ code: `export default class MyClass {}` }), +describe(`no-anonymous-exports-page-templates`, () => { + ruleTester.run(`passes valid and invalid cases`, rule, { + valid: [ + // Exports with identifiers are valid + test({ code: `const Named = () => {}\nexport default Named` }), + test({ code: `export default function foo() {}` }), + test({ code: `export default class MyClass {}` }), - // Sanity check unrelated export syntaxes - test({ code: `export * from 'foo'` }), - test({ code: `const foo = 123\nexport { foo }` }), - test({ code: `const foo = 123\nexport { foo as default }` }), + // Sanity check unrelated export syntaxes + test({ code: `export * from 'foo'` }), + test({ code: `const foo = 123\nexport { foo }` }), + test({ code: `const foo = 123\nexport { foo as default }` }), - // Allow call expressions by default for backwards compatibility - test({ code: `export default foo(bar)` }), - ], - invalid: [ - test({ - code: `export default () => {}`, - errors: [{ messageId: `anonymousArrowFunction` }], - }), - test({ - code: `export default function() {}`, - errors: [{ messageId: `anonymousFunctionDeclaration` }], - }), - test({ - code: `export default class {}`, - errors: [{ messageId: `anonymousClass` }], - }), - ], + // Allow call expressions by default for backwards compatibility + test({ code: `export default foo(bar)` }), + ], + invalid: [ + test({ + code: `export default () => {}`, + errors: [{ messageId: `anonymousArrowFunction` }], + }), + test({ + code: `export default function() {}`, + errors: [{ messageId: `anonymousFunctionDeclaration` }], + }), + test({ + code: `export default class {}`, + errors: [{ messageId: `anonymousClass` }], + }), + ], + }) }) From 765986f344af2813c83c7c6ca1407a3fb362ed4f Mon Sep 17 00:00:00 2001 From: LekoArts Date: Fri, 15 Jan 2021 11:36:10 +0100 Subject: [PATCH 21/23] handle more complicated cases --- .../limited-exports-page-templates.ts | 33 ++++++-- .../limited-exports-page-templates.ts | 78 ++++++++++++++++--- 2 files changed, 94 insertions(+), 17 deletions(-) diff --git a/packages/gatsby/src/utils/eslint-rules/__tests__/limited-exports-page-templates.ts b/packages/gatsby/src/utils/eslint-rules/__tests__/limited-exports-page-templates.ts index d2a28cf77cf67..0086df1c0d108 100644 --- a/packages/gatsby/src/utils/eslint-rules/__tests__/limited-exports-page-templates.ts +++ b/packages/gatsby/src/utils/eslint-rules/__tests__/limited-exports-page-templates.ts @@ -23,30 +23,51 @@ describe(`no-anonymous-exports-page-templates`, () => { ruleTester.run(`passes valid and invalid cases`, rule, { valid: [ test({ - code: `const Template = () => {}\nexport const query = graphql\`test\`\nexport default Template`, + code: `import { graphql, Link } from "gatsby"\nconst Template = () => {}\nexport const query = graphql\`test\`\nexport default Template`, + }), + test({ + code: `import { graphql as wat, Link } from "gatsby"\nconst Template = () => {}\nexport const query = wat\`test\`\nexport default Template`, + }), + test({ + code: `import * as Gatsby from "gatsby"\nconst Template = () => {}\nexport const query = Gatsby.graphql\`test\`\nexport default Template`, }), test({ code: `const Template = () => {}\nexport default Template`, }), test({ - code: `const Template = () => {}\nconst query = graphql\`test\`\nexport { query }\nexport default Template`, + code: `import graphql from "graphql-tag"\nconst Template = () => {}\nconst stuff = graphql\`test\`\nexport default Template`, + }), + test({ + code: `import { graphql } from "gatsby"\nconst Template = () => {}\nconst query = graphql\`test\`\nexport { query }\nexport default Template`, }), ], invalid: [ test({ - code: `const Template = () => {}\nexport const query = graphql\`test\`\nexport function Test() {}\nexport default Template`, + code: `import { graphql } from "gatsby"\nconst Template = () => {}\nexport const query = graphql\`test\`\nexport function Test() {}\nexport default Template`, + errors: [{ messageId: `limitedExportsPageTemplates` }], + }), + test({ + code: `import graphql from "graphql-tag"\nconst Template = () => {}\nexport const query = graphql\`test\`\nexport default Template`, + errors: [{ messageId: `limitedExportsPageTemplates` }], + }), + test({ + code: `import { graphql as wat } from "gatsby"\nimport graphql from "graphql-tag"\nconst Template = () => {}\nexport const query = wat\`test\`\nexport const query2 = graphql\`test2\`\nexport default Template`, + errors: [{ messageId: `limitedExportsPageTemplates` }], + }), + test({ + code: `import { graphql } from "gatsby"\nconst Template = () => {}\nconst query = graphql\`test\`\nfunction Test() {}\nexport { query, Test }\nexport default Template`, errors: [{ messageId: `limitedExportsPageTemplates` }], }), test({ - code: `const Template = () => {}\nconst query = graphql\`test\`\nfunction Test() {}\nexport { query, Test }\nexport default Template`, + code: `import { graphql } from "gatsby"\nconst Template = () => {}\nexport const query = graphql\`test\`, hello = 10\nexport default Template`, errors: [{ messageId: `limitedExportsPageTemplates` }], }), test({ - code: `const Template = () => {}\nexport const query = graphql\`test\`, hello = 10\nexport default Template`, + code: `import * as Gatsby from "gatsby"\nconst Template = () => {}\nexport const query = Gatsby.graphql\`test\`, hello = 10\nexport default Template`, errors: [{ messageId: `limitedExportsPageTemplates` }], }), test({ - code: `const Template = () => {}\nexport const hello = 10, query = graphql\`test\`\nexport default Template`, + code: `import { graphql } from "gatsby"\nconst Template = () => {}\nexport const hello = 10, query = graphql\`test\`\nexport default Template`, errors: [{ messageId: `limitedExportsPageTemplates` }], }), ], diff --git a/packages/gatsby/src/utils/eslint-rules/limited-exports-page-templates.ts b/packages/gatsby/src/utils/eslint-rules/limited-exports-page-templates.ts index 29039cf9d4e50..d0678434c38fa 100644 --- a/packages/gatsby/src/utils/eslint-rules/limited-exports-page-templates.ts +++ b/packages/gatsby/src/utils/eslint-rules/limited-exports-page-templates.ts @@ -1,8 +1,15 @@ import { Rule } from "eslint" -import { Node, Identifier } from "estree" +import { + Node, + Identifier, + ImportDeclaration, + TaggedTemplateExpression, +} from "estree" import { store } from "../../redux" import { isPageTemplate } from "../eslint-rules-helpers" +const DEFAULT_GRAPHQL_TAG_NAME = `graphql` + function hasOneValidNamedDeclaration( node: Node, varName: string | undefined @@ -26,7 +33,11 @@ function hasOneValidNamedDeclaration( return false } -function isTemplateQuery(node: Node): boolean { +function isTemplateQuery( + node: Node, + graphqlTagName: string, + namespaceSpecifierName: string +): boolean { // For export const query = 'foobar' the declaration exists with type 'VariableDeclaration' // Checks for: @@ -36,14 +47,29 @@ function isTemplateQuery(node: Node): boolean { // The array will have two items. So use every() to check if only one item exists // With TaggedTemplateExpression and "graphql" name + // In addition the declaration can also be a MemberExpression like + // Gatsby.graphql`` when the import happened with import * as Gatsby from "gatsby" + return ( node.type === `ExportNamedDeclaration` && node.declaration?.type === `VariableDeclaration` && - node.declaration?.declarations.every( - el => + node.declaration?.declarations.every(el => { + if ( el?.init?.type === `TaggedTemplateExpression` && - (el.init.tag as Identifier)?.name === `graphql` - ) + el.init.tag.type === `Identifier` + ) { + return el.init.tag.name === graphqlTagName + } else if ( + el?.init?.type === `TaggedTemplateExpression` && + el.init.tag.type === `MemberExpression` + ) { + return ( + (el.init.tag.object as Identifier).name === namespaceSpecifierName && + (el.init.tag.property as Identifier).name === DEFAULT_GRAPHQL_TAG_NAME + ) + } + return false + }) ) } @@ -54,7 +80,7 @@ const limitedExports: Rule.RuleModule = { limitedExportsPageTemplates: `In page templates only a default export of a valid React component and the named export of a page query is allowed. All other named exports will cause Fast Refresh to not preserve local component state and do a full refresh. - Please move your other named exports to another file. + Please move your other named exports to another file. Also make sure that you only export page queries that use the "graphql" tag from "gatsby". `, }, }, @@ -64,13 +90,43 @@ const limitedExports: Rule.RuleModule = { } let queryVariableName: string | undefined = `` + let graphqlTagName = `` + let namespaceSpecifierName = `` return { + ImportDeclaration: (node): void => { + // Make sure that the specifier is imported from "gatsby" + if ((node as ImportDeclaration).source.value === `gatsby`) { + const graphqlTagSpecifier = (node as ImportDeclaration).specifiers.find( + el => { + // We only want import { graphql } from "gatsby" + // Not import graphql from "gatsby" + if (el.type === `ImportSpecifier`) { + // Only get the specifier with the original name of "graphql" + return el.imported.name === DEFAULT_GRAPHQL_TAG_NAME + } + // import * as Gatsby from "gatsby" + if (el.type === `ImportNamespaceSpecifier`) { + namespaceSpecifierName = el.local.name + return false + } + return false + } + ) + if (graphqlTagSpecifier) { + // The local.name handles the case for import { graphql as otherName } + // For normal import { graphql } the imported & local name are the same + graphqlTagName = graphqlTagSpecifier.local.name + } + } + return undefined + }, TaggedTemplateExpression: (node): void => { if ( - node.type === `TaggedTemplateExpression` && - // @ts-ignore - node.tag?.name === `graphql` + (node as TaggedTemplateExpression).type === + `TaggedTemplateExpression` && + ((node as TaggedTemplateExpression).tag as Identifier)?.name === + graphqlTagName ) { if (queryVariableName) { return undefined @@ -86,7 +142,7 @@ const limitedExports: Rule.RuleModule = { return undefined } - if (isTemplateQuery(node)) { + if (isTemplateQuery(node, graphqlTagName, namespaceSpecifierName)) { return undefined } From 0fd9debcbe067c20bc8aeedb7d1b7c5385358c25 Mon Sep 17 00:00:00 2001 From: LekoArts Date: Fri, 15 Jan 2021 11:39:56 +0100 Subject: [PATCH 22/23] add one more case --- .../eslint-rules/__tests__/limited-exports-page-templates.ts | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/packages/gatsby/src/utils/eslint-rules/__tests__/limited-exports-page-templates.ts b/packages/gatsby/src/utils/eslint-rules/__tests__/limited-exports-page-templates.ts index 0086df1c0d108..ec02b44ae4fe0 100644 --- a/packages/gatsby/src/utils/eslint-rules/__tests__/limited-exports-page-templates.ts +++ b/packages/gatsby/src/utils/eslint-rules/__tests__/limited-exports-page-templates.ts @@ -46,6 +46,10 @@ describe(`no-anonymous-exports-page-templates`, () => { code: `import { graphql } from "gatsby"\nconst Template = () => {}\nexport const query = graphql\`test\`\nexport function Test() {}\nexport default Template`, errors: [{ messageId: `limitedExportsPageTemplates` }], }), + test({ + code: `import graphql from "gatsby"\nconst Template = () => {}\nexport const query = graphql\`test\`\nexport default Template`, + errors: [{ messageId: `limitedExportsPageTemplates` }], + }), test({ code: `import graphql from "graphql-tag"\nconst Template = () => {}\nexport const query = graphql\`test\`\nexport default Template`, errors: [{ messageId: `limitedExportsPageTemplates` }], From 2252bf7dd9f5aeec5b26fc9394b0ec6f0e9925fb Mon Sep 17 00:00:00 2001 From: LekoArts Date: Fri, 15 Jan 2021 14:59:53 +0100 Subject: [PATCH 23/23] handle commonjs syntax --- .../limited-exports-page-templates.ts | 12 +++++ .../limited-exports-page-templates.ts | 44 +++++++++++++++++++ 2 files changed, 56 insertions(+) diff --git a/packages/gatsby/src/utils/eslint-rules/__tests__/limited-exports-page-templates.ts b/packages/gatsby/src/utils/eslint-rules/__tests__/limited-exports-page-templates.ts index ec02b44ae4fe0..6859772206217 100644 --- a/packages/gatsby/src/utils/eslint-rules/__tests__/limited-exports-page-templates.ts +++ b/packages/gatsby/src/utils/eslint-rules/__tests__/limited-exports-page-templates.ts @@ -25,6 +25,18 @@ describe(`no-anonymous-exports-page-templates`, () => { test({ code: `import { graphql, Link } from "gatsby"\nconst Template = () => {}\nexport const query = graphql\`test\`\nexport default Template`, }), + test({ + code: `const { graphql } = require("gatsby")\nconst Template = () => {}\nexport const query = graphql\`test\`\nexport default Template`, + }), + test({ + code: `const { graphql } = require(\`gatsby\`)\nconst Template = () => {}\nexport const query = graphql\`test\`\nexport default Template`, + }), + test({ + code: `const { graphql: wat } = require("gatsby")\nconst Template = () => {}\nexport const query = wat\`test\`\nexport default Template`, + }), + test({ + code: `const { graphql: wat } = require(\`gatsby\`)\nconst Template = () => {}\nexport const query = wat\`test\`\nexport default Template`, + }), test({ code: `import { graphql as wat, Link } from "gatsby"\nconst Template = () => {}\nexport const query = wat\`test\`\nexport default Template`, }), diff --git a/packages/gatsby/src/utils/eslint-rules/limited-exports-page-templates.ts b/packages/gatsby/src/utils/eslint-rules/limited-exports-page-templates.ts index d0678434c38fa..9284bb7645e07 100644 --- a/packages/gatsby/src/utils/eslint-rules/limited-exports-page-templates.ts +++ b/packages/gatsby/src/utils/eslint-rules/limited-exports-page-templates.ts @@ -4,6 +4,12 @@ import { Identifier, ImportDeclaration, TaggedTemplateExpression, + VariableDeclaration, + CallExpression, + Literal, + TemplateLiteral, + VariableDeclarator, + ObjectPattern, } from "estree" import { store } from "../../redux" import { isPageTemplate } from "../eslint-rules-helpers" @@ -94,6 +100,44 @@ const limitedExports: Rule.RuleModule = { let namespaceSpecifierName = `` return { + // const { graphql } = require('gatsby') + VariableDeclaration: (node): void => { + // Check if require('gatsby') + const requiredFromGatsby = (node as VariableDeclaration).declarations.find( + el => { + // Handle require(`gatsby`) + if ( + (el.init as CallExpression)?.arguments?.[0]?.type === + `TemplateLiteral` + ) { + return ( + ((el.init as CallExpression).arguments[0] as TemplateLiteral) + ?.quasis[0].value.raw === `gatsby` + ) + } + + return ( + ((el.init as CallExpression)?.arguments?.[0] as Literal) + ?.value === `gatsby` + ) + } + ) + + if (requiredFromGatsby) { + // Search for "graphql" in a const { graphql, Link } = require('gatsby') + const graphqlTagSpecifier = ((requiredFromGatsby as VariableDeclarator) + .id as ObjectPattern)?.properties.find( + el => (el.key as Identifier).name === DEFAULT_GRAPHQL_TAG_NAME + ) + + if (graphqlTagSpecifier) { + graphqlTagName = (graphqlTagSpecifier.value as Identifier).name + } + } + + return undefined + }, + // import { graphql } from "gatsby" ImportDeclaration: (node): void => { // Make sure that the specifier is imported from "gatsby" if ((node as ImportDeclaration).source.value === `gatsby`) {