Create React App - linting & CI/CD

[ttmarek]

• This is not a bug report.
• This is a series of questions I had when trying to figure out linting and CI/CD in CRA. There are potential issues we can discuss here as well.
• This project is amazing, and I really appreciate all the hard work that has been put into it. ❤️

I may be wrong, but there looks to be a number of discrepancies between the User Guide and CRA's actual behaviour with regard to linting.

In the Continuous Integration section it says:

When creating a build of your application with npm run build linter warnings are not checked by default. Like npm test, you can force the build to perform a linter warning check by setting the environment variable CI. If any warnings are encountered then the build fails.

But if I run CI=false npm run build it still shows linter warnings. And, maybe I missed it, but there doesn't seem to be any checks for the CI environment variable in webpack.config.prod.js or webpack.config.prod.js. The linter seems to run by default on every build.

Towards the end of the section it also says:

The build command will check for linter warnings and fail if any are found.

The build command does check for linter warnings. But it won't fail on a linter warning. Should this read. "The build command will check for linter errors and fail if any are found."?

This brings me to something else that's giving me trouble. In eslint-config-react-app/index.js there is a comment at the top that says:

We use eslint-loader so even warnings are very visible. This is why we only use "WARNING" level for potential errors, and we don't use "ERROR" level at all.

Okay....but if you scroll down to the eslint rules there are a number of rules that are defined at the error level. Namely, no-undef, no-restricted-globals, no-restricted-properties, import/first, import/no-amd, import/no-webpack-loader-syntax...and more.

Looking deeper into eslint-config-react-app/index.js made me even more confused. For context, I had just read through issue #2157. And particularly @gaearon's comment, where it states:

Our rules are specifically picked to not enforce style, and to only find logical mistakes.

and

We think ESLint rules are a fundamentally flawed way to enforce coding style, and we suggest using Prettier for that instead.

But there are a number of rules in there that are pretty style-specific. To prove it you can even do an intersect between the rules defined here and those found in eslint-config-prettier:

0: "dot-location"
1: "new-parens"
2: "no-mixed-operators"
3: "no-whitespace-before-property"
4: "rest-spread-spacing"
5: "unicode-bom"

Some Thoughts

I think I understand the sentiment behind the comment in eslint-config-react-app/index.js where it says that using eslint-loader should make linter warnings visible enough that there doesn't need to be linter errors. This is great for beginners that might otherwise get discouraged by the inability to build their new React app because of a linter errors. The linter is there for them as a learning aid, but it doesn't act as a blocker.

But, CRA is too useful a tool to be used solely by beginners on their weekend project. And there is a real need in a professional setting to turn those linter "warnings" into "errors", and have them fail in CI. What do y'all think about adding a flag or something to turn those warnings into errors? I can volunteer some of my time to do this myself if there is interest.

Also, It seems like the only way to run these lint rules is by building the app. Is there a reason behind this? Many enterprises are starting to use CRA, and many pay by build minutes. So this isn't just an issue of having to wait for a build in order to have some lint checks, this decision is costing businesses money. What if there were a separate "lint" react script that we could provide "--warn" (default) or "--error" flags to run in CI?

To Summarize

• the docs say the linter isn't run by default on every build, but it looks like it is.
• the docs say the build will fail on linter warnings, but it doesn't. (it does if CI=true)
• a comment in eslint-config-react-app says there shouldn't be any rules set at an "error" level, but there are.
• it seems like the lint rules provided by CRA were supposed to not be about styling. But it looks like a few of them are.
• from what I understand the only way to run these lint rules is by building the app, this isn't ideal in some contexts.

[Timer]

I'll go through this issue more in a bit, but setting CI=true should make the build fail if warnings are encountered.

[ttmarek]

@Timer you're right! Perfect. So that takes care of a bunch of my confusion. I just misunderstood part of the docs. I edited my original comment accordingly.

[Timer]

Great! & like I said I will go through this issue a bit more later tonight.

If you found the documentation confusing around CI, would you please care to submit a pull request clarifying it? 😄

[ttmarek]

Great! & like I said I will go through this issue a bit more later tonight.

Yeah no rush m8, this isn't a blocker.

If you found the documentation confusing around CI, would you please care to submit a pull request clarifying it?

Absolutely. At the very minimum I'll adjust the wording per our discussion.

[Timer]

We use eslint-loader so even warnings are very visible. This is why we only use "WARNING" level for potential errors, and we don't use "ERROR" level at all.

So we only use the error level for things we are 100% confident are errors.

For example, we set no-undef to the error level because it's not useful to only warn about an actual problem; we warn about potential problems.

The import/* rules are extensions of restrictions within our system, as we do not support webpack AMD syntax or loaders -- so these are errors [because they actually are].

Originally, we may have never had errors. I think it'd be better to update this comment and say we error for things which are actual errors or unsupported functionality which would result in an error / unpredictable behavior.

Also, It seems like the only way to run these lint rules is by building the app. Is there a reason behind this?

That is true currently, but there's a pull request to add a lint command. 😄
In the beginning we didn't see a reason for it, but the pay-per-minute builds provided us with a good reason.

the docs say the linter isn't run by default on every build, but it looks like it is.

How did you come to this conclusion?

it seems like the lint rules provided by CRA were supposed to not be about styling. But it looks like a few of them are.

A few are! And we generally take pull requests to remove them. Very few however are style-esque because they almost always are a mistake.