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 1 commit
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
57 changes: 52 additions & 5 deletions packages/gatsby/src/utils/eslint-config.ts
Original file line number Diff line number Diff line change
@@ -1,26 +1,73 @@
import { printSchema, GraphQLSchema } from "graphql"
import { CLIEngine } from "eslint"

const eslintRulePaths = `${__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 {
existingOptions.baseConfig.extends = [eslintRequirePreset]
}
}

export const eslintConfig = (
schema: GraphQLSchema,
usingJsxRuntime: boolean
): CLIEngine.Options => {
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
7 changes: 7 additions & 0 deletions packages/gatsby/src/utils/eslint/required.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
module.exports = {
rules: {
// Custom ESLint rules from Gatsby
"no-anonymous-exports-page-templates": `warn`,
"limited-exports-page-templates": `warn`,
},
}
46 changes: 43 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,38 @@ export function reactHasJsxRuntime(): boolean {

return false
}

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 => {
if (typeof rule.loader === `string`) {
return rule.loader.includes(`eslint-loader`)
Copy link
Contributor

Choose a reason for hiding this comment

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

Includes? No risk of false positives here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

maybe I can keep includes('eslint-loader') as "precheck" and if those are satisfied do a deeper check (i.e. read package.json of imported package and see if package actually is eslint-loader), but no matter how I slice it - it will be hacky :(

Copy link
Contributor

Choose a reason for hiding this comment

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

I think filename based validation is fine here. But a substr check seems a little too much of a shortcut, that's all.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmmm, so do you have any advice on what would good to do here?

I can cover maybe few specific scenario:

  • loader is defined by package name (not fully resolved) - so for this I would do strict comparison to eslint-loader (no includes)
  • loader has some resolved path (so either eslint-loader/index.js or eslint-loader/dist/cjs.js) - then I can do includes or regex matching those

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 hopefully more strict checks in f477549 which I don't particularly like (but neither I did includes('eslint-loader')) as it's hacky no matter how I slice it

}

return false
})

if (rule) {
if (typeof rule.options !== `string`) {
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
}