Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(gatsby): add required eslint rules even if user has custom eslint config #28911

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
130 changes: 130 additions & 0 deletions packages/gatsby/src/utils/__tests__/eslint-config.ts
Original file line number Diff line number Diff line change
@@ -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",
}
`)
})
})
})
66 changes: 61 additions & 5 deletions packages/gatsby/src/utils/eslint-config.ts
Original file line number Diff line number Diff line change
@@ -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) &&
Comment on lines +40 to +42
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So if .extends exists but is not an array then the eslintRequirePreset part is silently dropped? Smells like a bit like a footgun.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right, I thought extends only accept arrays but it also accept just strings, so I will add handling for it

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added handling for single string case in f5ad45a + some tests for it

!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,
Expand All @@ -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`.
Expand Down
9 changes: 9 additions & 0 deletions packages/gatsby/src/utils/eslint/required.js
Original file line number Diff line number Diff line change
@@ -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`,
},
}
58 changes: 55 additions & 3 deletions packages/gatsby/src/utils/webpack-utils.ts
Original file line number Diff line number Diff line change
@@ -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"
Expand All @@ -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<T = {}> = (options?: T) => Loader

Expand Down Expand Up @@ -124,6 +128,8 @@ interface IWebpackUtils {
plugins: PluginUtils
}

const vendorRegex = /(node_modules|bower_components)/

/**
* A factory method that produces an atoms namespace
*/
Expand All @@ -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`)
Expand Down Expand Up @@ -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
}
14 changes: 12 additions & 2 deletions packages/gatsby/src/utils/webpack.config.js
Original file line number Diff line number Diff line change
Expand Up @@ -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"

Expand Down Expand Up @@ -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
}