Improving the ESLint experience

[mrmckeb]

We want to thank everyone for their feedback on the ESLint changes in Create React App v4. We want to make this experience more flexible, and wanted to gather more feedback from the community before making changes.

We understand that for some users, this change meant more errors showing in our error overlay, and for others, blocking builds.

To help us prioritise improvements, we'd like to hear from you. Vote with 👀 if you'd like the option to omit ESLint errors from error overlay, and/or 🚀 if you'd like the option to disable ESLint errors and warnings during builds.

If you have an other thoughts, please let us know in the comments below!

Related issues: #9887, #9929
Related PR: #10170

[nabilfreeman]

Thank you for opening up this issue and coming back to the community. Is there a philosophy why ESLint itself shouldn't be an optional feature in the build process?

In addition to the recent breaking builds I've also run into trouble with ESLint and CRA before when trying to customise its rules to ones that fit the organisation I'm working with.

The main reason that comes to mind is possibly the CRA team wanting to keep changes in environment as minimal as possible so the library is stable and easy to debug.

[mrmckeb]

Some of these decisions were made before the time of the current core team, but we do try to "rethink" things when we can. So I can't tell you exactly why.

We're definitely open to any ideas on how to improve this workflow - thanks for your input!

[xfournet]

Thanks for creating this discussion. Indeed eslint during build is a pain in the ass. Except maybe for unexperienced developer's, it's counter productive.

Our context/setup:

• we have our own eslint configuration (.eslintrc.json based on airbnb and airbnb/hooks plus our some customizations)
• we disabled the SKIP_PREFLIGHT_CHECK (SKIP_PREFLIGHT_CHECK=true in .env) because we want to use the same eslint (and plugins) version across all our JS code (we have other JS modules which are not React)
• Intellij IDEA -> eslint errors are already in the IDE
• we are hacking on the fly the react-script webpack config to remove the ESLintWebpackPlugin so we are not bothered by eslint during build (and build speed is correct)
• we added a lint script in our package.json which can be called by developer's and which is enforced on CI ("lint": "eslint --max-warnings 0 .")

Adding an option to completely disable eslint handling by CRA would help us. This option would permit:

• no check of eslint versions in preflight check (so we don't have to disable all preflight checks)
• no ESLintWebpackPlugin in webpack.config (so we don't have to hack on the fly the webpack config generated by react-script)

[mrmckeb]

We had discussions around this, it might be the simplest path. Thanks for sharing and providing detailed context.

I also use my IDEA (VSCode), Git hooks, and the CI to check linting.

[dwelle]

It should be noted that by

if you'd like the option to disable ESLint errors and warnings during builds.

it is meant that you will be able to disable the errors only for specific environments, such as development, while still being able to keep them in production.

I also assume that you would be able to disable just warnings separately, and retain errors, correct?

[mrmckeb]

Not quite, from the reported issues it seems that for some users errors are more of a concern (during development), and then other users don't want errors or warnings to affect builds (production).

[jrr]

I mostly use ESLint 1) in my editor, and 2) on the command-line in CI.

The only time I don't mind CRA running lint for me is in a production-mode build, e.g., react-scripts build. I don't need it to, but it's fine.

The rest of the time, it's not just that I don't want errors to appear -- I don't want to pay the performance cost at all.

Reasons I don't want to always pay the performance cost:

• We use a lot of lint rules. A full project lint currently takes 28s on a beefy MacBook Pro. When I'm adding lint rules for safety and code quality, I don't want to worry about how much it will slow anything down.
• On my project, humans are not the only users of the dev server. We also use it for browser integration testing (with a real backend and a real database). Every bit of performance optimization helps, especially on feeble CI agents.

(so, for my case, the current ESLINT_BUILD_ON_ERROR idea wouldn't be enough)

Don't get me wrong, I think the dev server integration is a cool feature, especially for the out-of-the-box experience for a new developer on a new project. It just loses its utility as the project grows, so it ought to be disable-able without hacks.

[mrmckeb]

If we do got with an ESLINT_BUILD_ON_ERROR it won't run the check at all, it will just bypass it. The PR was reworked from that implementation, and I forgot to rename that variable - which is why (as @ianschmitz highlighted) it is confusing.

The var should have been something like ESLINT_FORCE_WARNINGS... again, sorry for the confusion.

Thanks for your response, and the detail you've provided.

[mlarcher]

It seems to me that what we need is not anymore "FORCE_WARNINGS" than "BUILD_ON_ERROR", but rather "ESLINT_DISABLE". There's no point in running eslint checks at all during build step in a pipeline that has already done it during previous steps. Just running the checks can be time consuming, we need to be able to opt out of it entirely for dev server and production builds.

[dennisc9]

For our use case we'd like to suppress any warnings or errors in our production builds. eslint-loader has an option called quiet that could suppress this.

[bennettdams]

If you have an other thoughts, please let us know in the comments below!

I would like to refer to my proposal from a few months ago, which asks for the recommended way of config integration with ESLint, TypeScript, Prettier and VS Code. It has a 65 "thumbs up", so this is maybe something you could look into.

The two main questions are:

• What do I need to configure manually in the files of my CRA project? (e. g. .eslintrc.js) Do I need additional dependencies? How to integrate other ESLint plugins?
• What do I need to configure manually in my VS Code setup? (e. g. for the ESLint plugin (dbaeumer.vscode-eslint or the Prettier plugin)

These are questions a lot of people have and it would be nice to have an "official" documentation on the CRA website.

[mrmckeb]

Thanks for all of the valuable input here. We've decided to address both issues at once. PR #10170 has been updated to reflect this.

[thisismydesign]

Thanks for your work on this!

When trying the new flags I ran into the issue that I must use typescript eslint dependencies in my non-typescript project in order to extend the base config: #10502

Would be great to just have to extend "react-app" without adding seemingly unneeded dependencies. The package could even juggle those dependencies in the background if that's possible, but having them explicit is weird.

[mrmckeb]

Thanks for raising the issue @thisismydesign. We can discuss there.

[ClementParis016]

Thanks for bringing up this discussion.

Looking at the different feedbacks, I realise that I have an other concern about the ESLint experience.

I'm fine with having the ESLint plugin running, as long as it doesn't hurt build performance too much, but the thing I'm very annoyed by is the fact that ESLint warnings are treated as errors on CI builds:

create-react-app/packages/react-scripts/scripts/build.js

Lines 190 to 203 in 3f5dea9

	if (
	process.env.CI &&
	(typeof process.env.CI !== 'string' ||
	process.env.CI.toLowerCase() !== 'false') &&
	messages.warnings.length
	) {
	console.log(
	chalk.yellow(
	'\nTreating warnings as errors because process.env.CI = true.\n' +
	'Most CI servers set it automatically.\n'
	)
	);
	return reject(new Error(messages.warnings.join('\n\n')));
	}

This is honestly completely unrealistic. Most production-ready projects live with ESLint warnings (because technical debt, because sometimes you need a temporary hack, etc...) and failing the build because of those is counterproductive.

I get that we can still do CI=false react-scripts build but there might be other tools that need the CI flag to be true.
IMHO, there should be a dedicated arg for this setting, and the default behaviour should be to not fail on ESLint warnings (default behaviour is more debatable, I understand you might want to enforce new projects to start clean).
Would be something like that: react-scripts build --failOnLintWarnings=<true|false>

[mrmckeb]

HI @ClementParis016, you can now use DISABLE_ESLINT_PLUGIN to disable the plugin completely. I think/hope that should solve this issue 🙂

This is honestly completely unrealistic. Most production-ready projects live with ESLint warnings (because technical debt, because sometimes you need a temporary hack, etc...) and failing the build because of those is counterproductive.

How you handle this is very much a the decision of you/your team, and that's why we've added the new flags.

As an example, teams in my company don't push/merge code that fails linting, but the way we sometimes get there is by using disable comments for rules and TODO comments.

Every team has different workflows, so we want to try to cater for that - whilst still keeping CRA as simple as possible.