Add ESLint extend support to eslint-loader

[mrmckeb]

This allows users to extend the base ESLint config, so long as it is extending the react-app config.

Config in package.json
Initially, this is limited to users that use package.json to configure ESLint. We can extend this later, but it will have performance implications as we would need to:

• Find which config file the user is using.
• Read that file, and confirm that it extends the react-app config.
• Set that config to eslint-loader.

Eslint provides tooling to support this, but again, this will impact performance.

Users could work around this, by setting the following in package.json:

{
 "eslintConfig": {
   "extends": ["react-app", "./my-eslint-config.js"]
 }
}

Documentation
Once this approach is approved, I'll update our ESLint documentation to outline usage.

[gaearon]

The reason config wasn't configurable in the first place is to prevent hyper-opinionated configs full of styling rules from showing up in the browser overlay integration.

If we allow arbitrary configs we should make sure that this is still the case. E.g. at least adding Airbnb config shouldn't make a wrong indentation appear as a build error.

[gaearon]

Maybe we could force the linter to always have eslint-config-prettier applied on top so that styling rules are still force-disabled. However, there are still a bunch of rules in popular configs that are too restrictive, and we want to make it hard to enable them in the default setup.

[ianschmitz]

How do users override TypeScript rules? That was the main driver of this change.

[bradzacher]

If you're enforcing that users still include react-app then how come you don't just enable it by default instead of making people add it in when they extend?
Seems less error prone for the user if you do something like
if (!extends.includes('react-app')) { extends.unshift('react-app') }

Whilst it's not technically required, pretty much every plugin (and base eslint rules definitely do) follows the exported config structure defined by eslint:
https://eslint.org/docs/developer-guide/working-with-rules#rule-basics

This means that every rule should have a meta.type field on its definition which can help you figure out what should be shown in the overlay.
The value should be one of "problem" | "suggestion" | "layout".

I would just setup the hard rule that a plugin must have type set to "problem" to show up in the overlay. But you might want to review the rules you've got enabled by default to double check that.

Similarly there is a docs.category field, but that one is a free string, so you can't really rely upon its value. A lot of plugins follow eslint's lead and go with something along the lines of 'Best Practices' | 'Stylistic Issues' | 'Variables' | 'Possible Errors'

[gaearon]

I would just setup the hard rule that a plugin must have type set to "problem" to show up in the overlay.

This sounds reasonable to me.

[ianschmitz]

Great suggestions, @bradzacher. Thanks!

If users don't extend react-app in their config, they won't get proper ESLint extension integration, but i guess we can cover that in the docs.

[mrmckeb]

@gaearon I think we're all on the same page. I personally don't like breaking builds unless it's a serious issue, but we're trying to allow people to have more control over this.

We can use recommend people to set warn for anything that shouldn't break the build.

@bradzacher, as @ianschmitz mentioned, it's about consistency with IDE integration. I'd prefer that users know what's going on - because they explicitly set that config - rather than us adding things in the background.

I'd be interested to hear more about your idea @bradzacher, as I can't see a simple way to configure that in eslint-loader or ESLint itself. We don't want to be passing all rules.

[bradzacher]

I was weighing in from a linting perspective! I don't have much of an idea of CRA/Webpack.

I had a quick look and there's a lot of codepaths - I'm not sure how it would work within this project.
(pure JS at this scale is hard to figure out exactly without types or codebase knowledge...)

With my lack of knowledge of what CRA error output looks like (I've never properly used CRA), it looks like it could be a bit of a larger change across a number of files?
I wasn't sure how the errors get passed via the socket to your webpack client (is this an automatic webpack thing?)

[mrmckeb]

Sorry @bradzacher, I misunderstood and thought you meant we could configure ESLint that way.

I think it would be a fairly large change... And I'm not sure of the performance and maintainability impacts it might have. @gaearon @ianschmitz What do you think?

[Asday]

@gaearon not wanting opinionated linting to break builds is an opinion, though. On my project, I want to enforce code style, and have good reasons to believe it should be on save rather than on commit which I don't think I need to go into here.

CRA is awesome, but if I want to lint aggressively, I need to maintain a fork, or eject, both options of which are way over the top for something so simple.

At least give us the option.

[caub]

Thanks for this PR, this is useful when you migrate from an older codebase (using CRA v1 for example), and need to convert a few eslint errors to warnings, just to npm run dev at least

@mrmckeb Could we allow also an .eslintrc (jsonc, the 'c' is for comments that are allowed) file?
Not a big deal though, it'd be ok in package.json

[Asday]

As @caub says, except also .eslintrc.js - because I like commenting why a rule is there.

[dannycochran]

The reason config wasn't configurable in the first place is to prevent hyper-opinionated configs full of styling rules from showing up in the browser overlay integration. If we allow arbitrary configs we should make sure that this is still the case. E.g. at least adding Airbnb config shouldn't make a wrong indentation appear as a build error.

@gaearon Isn't this configurable by the developer? Eslint rules differentiate between warning or error (1 or 2), so a developer gets to choose what shows up in the overlay? I think that's currently how it's set up now: e.g. exhaustive-deps shows up as a warning in the browser console, not as an error in the app overlay.

Maybe we could force the linter to always have eslint-config-prettier applied on top so that styling rules are still force-disabled.

I also don't think we should mandate prettier usage. The VSCode extension is actually currently broken which makes it very difficult to use while working locally.

In general, I don't understand why CRA cares what a user does with their codebase after initialization, and should only provide guard rails for things that would break other parts of the CRA build tools. For instance, we cannot import files outside the src directory. This is a necessary opinion for CRA to function properly. But I can add custom linting rules and it has no impact on the rest of the toolchain, as far as I know.

IMO trying to mandate linting rules is no different than mandating usage of, say, RXJS instead of Redux. This seems out of scope for CRA.

@caub thank you for doing this. I think it would be good to support .eslintrc and .eslintrc.js, per @Asday 's comment, but that could certainly be done in a followup to at least unblock folks for now.

[bradzacher]

The VSCode extension is actually currently broken which makes it very difficult to use while working locally.

@dannycochran - The VSCode extension isn't technically broken, prettier-eslint is broken (using the option prettier.eslintIntegration switches the extension to use prettier-eslint instead of prettier directly).

Note that prettier and prettier-eslint are separate tools, the latter tool integrates the two tools together at fix time.

Also note that if you install the eslint extension, turn off prettier.eslintIntegration, and turn on eslint.autoFixOnSave - it will work around the issue.

[mrmckeb]

@caub We could do that for sure. We could release without and perhaps drop it in as an update - I think at this stage we need to get something out, and we can then "enhance" once we've proven that it's working.

[silverwind]

I agree that forcing the configuration via package.json for eslint configuration is not ideal. Some editor integrations will not properly parse package.json and some of them are so dumb that they only support .eslintrc and nothing else.

I suggest letting eslint's own config resolution do all the work:

baseConfig - set this to eslint-config-react-app. If .eslintrc, exists, eslint will merge both configs with the user config taking precedence. Document that the user should extends: react-app in their config.
ignore - remove the option to enable vendoring JS libs in src without forcing a reformat.
useEslintrc - remove the option, otherwise .eslintrc will not work.

So basically, the config would just be baseConfig, optionally extended by the user to their liking. No special environment variables required and eslint will do all the config resolution itself.

[mrmckeb]

This PR was updated yesterday to do just that @silverwind (cool name btw), but I neglected to leave a comment.

[silverwind]

Ah I see. I think you need to update docs to remove the mentions of EXTEND_ESLINT which is gone.

Also would like to see ignore and eslintrc options removed for reasons mentioned above:

ignore: I don't really see a point of restricting users from being able to ignore some files in regards to linting.Some old-school JS libraries not available as modules are often only available in a minified format which is a pain to integrate without an option to ignore it from the linter.

eslintrc: I see this as essential for users to be free to choose where they config eslint, either a package.json prop or a standalone config file.

[mrmckeb]

@silverwind that was an oversight, corrected now. That env will be required until the next major release, where we may make this the default.

The config options are already updated.

The ignore issue isn't related to this PR, but worth raising. If you want to raise an issue about that, here's the original PR that introduced that. #2115 - I agree it's probably needed.

[CodingDive]

Hey, this PR sounds like an awesome addition to CRA but I'm wondering how does it compare to the existing way of extending ESLint? In the editor setup docs, it says the following:

add a file called .eslintrc.json to the project root:

{
 "extends": "react-app"
}

...
Note that even if you edit your .eslintrc.json file further, these changes will only affect the editor integration.

The ESLint configuration file can also (already) extend other configurations or plugins right?

[mrmckeb]

Hi @ReasonableDeveloper, this is nothing special in that sense - we're just requiring that people extend our config for now. We're not doing anything clever here, so you'll be able to work as you always have with ESLint.

[silverwind]

Creating a custom .eslintrc.json works for editor integrations because those don't use the webpack config but it's actually a footgun because react-scripts and webpack dev server will ignore the file and will lint differently (using their hardcoded config), until this PR which should actually allow custom config to work the same everywhere.