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

Support ESLint configure or disabling ESLint (discussion) #3886

Closed
oriSomething opened this issue Jan 21, 2018 · 26 comments
Closed

Support ESLint configure or disabling ESLint (discussion) #3886

oriSomething opened this issue Jan 21, 2018 · 26 comments

Comments

@oriSomething
Copy link

Sorry for removing the Github template, it was irrelevant. Anyway:

As continuation of my comment from #3815

I know this issue discussed in the past in many different issues, and yet, I think we should revisit the support of custom ESLint config by user or at least the ability to remove it so we can use our configuration or even in cases where users don't feel they need ESLint.

Good part with current state

  • Most of the configuration is good.
  • Config is config is config. If there is something the user can configure. It might break in future versions.

Pain points with current state

Currently the only option to really control the configuration of ESLint is by eject or by fork. In both cases we lose the charm of using react-scripts which makes most things to be more convenient and tools that are updated automagically between versions of packages such as webpack end ESLint as well.

But lack of controlling the configuration can cause a lot more issues, which makes the ESLint part in create-react-app to be a pain for development.

The eslint-config-react-app does contains stylistic rules, even tough they're not from the "prettier" kind. For example: default-case rule forces me to put empty default: case even when it's an empty and unneeded, since in my project the convention it to do the "default" after the "switch-case block" or in cases when default is really unneeded.
It's easy to say just add /* eslint-ignore default-case */, but it becoming more of a "file template" than some exception that happens "here and there".
Moreover, even most consensus rules such as eqeqeq might not feet to any project, and will be more of stylistic choice rather than the right one. For example, I worked on a project where I've used Flow and have a lot of cases where == was more than a necessary to make code readable, since when you're type is string | null | undefined and you need to make lots of compares things like a == b was much better than a === b || (a == null && b == null) (I've simplified the "If statement"). Flow can make sure it's not number compared to string (And I know, it's an edge case, but it was only for a demonstration).

Plugins are very useful, but when I can't config, it means no real use of plugins. Because, on the browser I'll have one configuration and on then IDE / CLI I'll have other configuration, which is a pain. There some cases you'll need to remove some ESLint rule because some plugin gives you a better rule which might conflict with the original rule. But you can't configure, so warnings / errors in the browser will be leave you with conflicts.
And The actual use of plugins is discourage since, as I said before, I'll get in the browser different result from what happens in the IDE / CLI.
Half related issue: #1345 (Why should I need to wait to the support or be forced to the plugin if I don't need?)

Globals are also a big issue, If you forced to use third party code from a CDN it means you'll have a lot of /* globals myAwesomeGlobal */ in your files. And please don't tell me to use window or global, It's a temporary work around not a solution.
In many cases when I'll want to remove the "global" since we stop to use some third party service, I won't have an easy way to know it "truly happens". When I'm not the only one who work on the project. Using window or global will only make it harder. Moreover, as browsers become more and more modern and some script will start use global as let or const the variable won't exist on the window object (not to mention when a file should run also inside Worker / Node which only makes things more complicated for using globals).
Moreover, there are libraries such as Modernizr 3.0 that force me to generate a file to src folder, that generated automatically. If I'll add it to public it will force another network request call and If I put it in src it forces me to add manually /* eslint-disable */ or start creating boilerplate scripts for this kind of code injection.
Relates issue: #2339 , #2863

Don't get me wrong, I fully understand the project philosophy and admire the work that have been done. But, on the other hand you lock me with this project "configuration" for a linter with specific rules which makes it harder work for me than not having the linter from the first place.
Since there is no easy way to opt-out without ejecting/forking, it makes it harder as more conflicts with the eslint-config-react-app happens during project life time.
Moreover, you actually do have configurations such as "homepage". Even if it's not much. ESLint is one of the biggest pain points for me in the create-react-app suite.

Suggestion

My suggestion is to allow custom configure or at least to disable ESLint project wide.

Related issue for disable ESLint: #2157
Related issue for allow custom config: #2318

P.S. I can find more related issues to every issue I've wrote here, but I don't think it gives any value.

@gaearon
Copy link
Contributor

gaearon commented Jan 21, 2018

Could you please extract specific pain points from the post in the form of a bullet list?

I don’t want to get into an argument about whether configuration is necessary/useful or not. I just want to see a specific list of perceived pain points, phrased in short sentences. For example:

  • Modernizr 3.0 generates a file that should be checked into src, but it wouldn’t lint correctly.

If you phrase your other pain points in a concise, specific way (instead of going into the philosophy of configuration) we can addesss them better.

@oriSomething
Copy link
Author

Some of the things I can't reveal specificly because of confidentiality.

But, I talked about specifics even if it's not "correct". For example:

  • the problem I have with globals because some third party code. Moreover I had this issue with injecting some globals in setupTests.js (Something I needed as a work around because of Jest bug not skiping tests with describe.skip in "check all files mode", which cause tests fail).
  • annoying rules such as default-case which populate the code base with empty default:
  • I can't use custom ESLint plugins that can be very helpful for a specific project that collaborated with other people.
  • no-unused-vars in the way I use it, is stricter than in eslint-config-react-app. Having two truths is annoying even if it's a minor issue.
  • Sometimes have problems with auto generated code by transpilers such as TypeScript CoffeeScript

I hope it's clearer (I believe it isn't because for most cases I can't reveal code).
But, TL;DR. The main issue is to be able to disable ESLint in all or part of the project. Preferable I'd like to have my custom configurations. But, it's not necessary as disabling it

@nfantone
Copy link

nfantone commented Feb 24, 2018

@oriSomething @gaearon Adding to the list:

  • Not being able to integrate Prettier with ESLint.
  • Having potentially different IDE / in-browser rules hurts developer experience.
  • Locking eslint + plugin versions force users to wait on next release for bug fixes.
  • Not being able to define a custom "lint" script to check for rules on CI jobs, such as "lint": "eslint src/**/*.js", even if it's only using default CRA setup (ie.: "extends": "react-app")
❯ yarn lint
yarn run v1.3.2
$ eslint src/**/*.js
Cannot find module 'eslint-config-react-app'
Referenced from: /dev/my-project/.eslintrc.json
Error: Cannot find module 'eslint-config-react-app'
Referenced from: /dev/my-project/.eslintrc.json
    at ModuleResolver.resolve (/dev/my-project/node_modules/eslint/lib/util/module-resolver.js:74:19)
...
error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.

Manually adding eslint-config-react-app to your dependencies leads to myriad of other problems + goes against the very philosophy of this project. Related: #3998


Proposal

  • Make eslint usage opt-in, but strongly recommended. Perhaps a "peerDependency"?
    • Or, make create-react-app generate the actual needed boilerplate to emulate current behaviour without hiding the configuration (i.e.: write a .eslintrc file on root + install and declare deps ala eject).
  • Let users extend configuration with whatever they (or their organizations) need/use.
  • Let users handle lint + plugins dependency versions.
  • Lint code during build/serving using local .eslintrc file.

The way I see it, enforcing "zero config" concepts on such opinionated/versatile tools like linters only harms the experience create-react-app is trying to provide. What's the real benefit with the current approach anyway? This is not a core component of the build pipeline. Locking versions and obscuring the works of trivial -but important- tasks like linters feels needlessly constricted, with very little trade-offs. IMHO, it's only there to save me from installing a bunch of dependencies and writing a configuration file. But writing the config (or rather, the need to do so) is what this issue is all about and the installation happens anyway. Both things can be greatly simplified by allowing create-react-app to output a suggested initial scaffolding.

@bugzpodder
Copy link

I keep a clean repo, and after upgrading: the no-unused-vars rule splits out a lot of warnings and is very annoying. Usually when I have an event handler, I don't need the event or when I do array.map i just want the index. There are tons of callsites and I do not enjoy putting an eslint-disable comment for these intentional use cases.

@ghost
Copy link

ghost commented Apr 16, 2018

CRA is opinionated. If you wanna ESLint configurations meeting your need, try react-app-rewire-eslint.

@bugzpodder
Copy link

It's pretty terrible that the CRA repro itself has to litter eslint-disable statements everywhere to satisfy its own eslint rules.

https://github.com/facebook/create-react-app/search?utf8=%E2%9C%93&q=no-unused-vars&type=

@nfantone
Copy link

Also terrible: nobody answering this thread.

@transitive-bullshit
Copy link
Contributor

Linting is imho CRA's biggest weak point at the moment.

My biggest issue with current CRA's handling of eslint is that it strictly enforces eslint as errors during the dev cycle, making it really annoying to quickly try stuff out while debugging. I always end up completely disabling CRA's eslint during development and rely on the linting of my editor of choice, which is a shame because linting with CRA shouldn't be so cringe-worthy.

I like @nfantone's proposal.

@calvellido
Copy link

I have a project with some customized rules:

(...)
    "rules": {
      "react/jsx-filename-extension": [1, {"extensions": [".js"]}],
      "react/jsx-no-duplicate-props": [1, { "ignoreCase": false }],
      "prettier/prettier": ["error", {
        "arrowParens": "always",
        "singleQuote": true,
        "trailingComma": "es5"
      }]
    },
(...)

I am using Material-UI-Next, where you can get some components like this (notice the two inputProps, InputProps, they are different things):

      <TextField
        type="number"
        name="sumInsured"
        label="Insured sum"
        value={sumInsured}
        inputProps={{ min: 0, max: 100000 }}
        InputProps={{ disableUnderline: true }}
        onChange={handleChange}
        className={styles.question}
      />

My npm scripts, and editor pick these rules correctly, hence not showing a warning on these components, but when running npm run build they are there...

This is very annoying, because I can't event create builds when process.env.CI is set to true 😞

@andrewnaeve
Copy link

+1 for not being able to use lint-staged on my mono-repo, if my frontend has create-react-app.

I wish there was a way to give precedence to configurations higher up in the tree.

@bugzpodder
Copy link

@andrewnaeve I have lint-staged and husky setup in my monorepo:
In the base directory, I have this in my package.json

"scripts": {
    "lint:staged": "lerna --stream --concurrency 1 run precommit",
  },
  "lint-staged": {
    "linters": {
      "*.{js,jsx,scss,json,md,config.js}": "yarn lint:staged"
    }
  },

and in each of the package.json in a workspace I have the standard lint-staged setup with concurrency: false and no husky installation.

@andrewnaeve
Copy link

@bugzpodder Wow thanks!

@mattfysh
Copy link

We're finding more and more rules which fall outside CRA's "errors only" and Prettier's "styles only", meaning we have no way to make use of them. e.g prettier/prettier#949

@gaearon what would you recommend here, e.g. submit a pull request switching this rule on in the next branch? The issue here is that there's no fallback if the PR Is rejected

@silverwind
Copy link

Workaround to use one's own .eslintrc without ejecting, persisting on npm install:

  "scripts": {
    "postinstall": "sed -i -E 's#\\[.*eslint-config-react-app.*\\]#['\\'$PWD/.eslintrc\\'']#g' node_modules/react-scripts/config/webpack.config.*.js"
  }

@bugzpodder
Copy link

I have my own .eslintrc which works in conjunction with CRA's eslintrc and it works pretty well for the past year:
#4684 (comment)

@silverwind
Copy link

So here is what I think CRA should do:

  • install eslint and eslint-config-react-app as a direct dependencies.
  • create a .eslintrc.js in the project root with just extends: eslint-config-react-app.
  • point webpack to above file. If it doesn't exist, use eslint-config-react-app.
  • enable normal usage of .eslintignore in webpack (doesn't respect .eslintignore #2339).

This should make everyone happy. Users get to extend the configuration and can install plugins and webpack and CLI/Editor linting are ran with this same linting configuration.

@corysimmons
Copy link

What @silverwind said except use https://github.com/davidtheclark/cosmiconfig so I can delete that .eslintrc and put "eslintConfig" in my package.json. 🤤

But yeah, CRA's linting rules are too weak to be useful.

I think the purpose of CRA should be to obfuscate Webpack junk while still exposing nice things like ESlint and Babel config. I was never really clear on why these 2 configs aren't exposed.

We'll get ESLint and Babel Issues incorrectly filed here

^ Are these going to outnumber the really valid "Can I please configure my code style...?" or "Can I add that Babel decorator plugin?" issues that get raised here every day?

Also, the community can help steer people to the right places for their issues, or hell, even just help people.

@nfantone
Copy link

eslint already supports configuration from package.json. No need for any extra sugar.

@Timer
Copy link
Contributor

Timer commented Sep 21, 2018

We enforce no style rules, if you find one we enforce by accident, please file a separate issue to have it disabled.

We're not ready to let people configure ESLint to apply any rules they desire yet, sorry!

@Timer Timer closed this as completed Sep 21, 2018
@crobinson42
Copy link

@Timer I would like to point out one very frustrating point - a lot of projects utilize css libraries like Bootstrap or Semantic-UI. When using a third party framework you have to follow their rules, one example:

<nav aria-label="Page navigation example">
    <ul className="pagination">
      <li className="page-item">
        <a className="page-link" href="#">
          <span>&laquo;</span>
          <span className="sr-only">Previous</span>
        </a>
      </li>
      <li className="page-item"><a className="page-link" href="#">1</a></li>
      <li className="page-item"><a className="page-link" href="#">2</a></li>
      <li className="page-item"><a className="page-link" href="#">3</a></li>
      <li className="page-item">
        <a className="page-link" href="#">
          <span>&raquo;</span>
          <span className="sr-only">Next</span>
        </a>
      </li>
    </ul>
  </nav>

The problem here is the create-react-app eslint config is lighting up my console with:

  Line 9:    The href attribute requires a valid address. Provide a valid, navigable address as the href value  jsx-a11y/anchor-is-valid
  Line 14:  The href attribute requires a valid address. Provide a valid, navigable address as the href value  jsx-a11y/anchor-is-valid
  Line 15:  The href attribute requires a valid address. Provide a valid, navigable address as the href value  jsx-a11y/anchor-is-valid
  Line 16:  The href attribute requires a valid address. Provide a valid, navigable address as the href value  jsx-a11y/anchor-is-valid
  Line 18:   The href attribute requires a valid address. Provide a valid, navigable address as the href value  jsx-a11y/anchor-is-valid

I realize that the html snippet is useless but In a more specific example I'm using onClick prop on my <a> elements and the only way to appease the eslint warning jsx-a11y/anchor-is-valid is to define a valid href, ie: href="#something" and I do not want a url hash value when a user clicks the element.

When create-react-app is making enforcements for the developer in their development environment, I think it should offer some configuration or disabling flag.

I'm sure folks can place many many more examples here of where create-react-app's eslint configs are either distracting with warnings (which takes away from warnings they might want to pay attention to) and/or we code around them which adds additional effort just to be sure warnings don't pop-up in our console during dev.

@russellr922
Copy link

from a mountain top: Prettier is not a linter. code quality rules !== formatting rules.

This is really still an issue (since 2016)? Linting should be done at compile and re-compile and OF COURSE every individual and/or organization will have different code quality rules!

How does enforcing your own eslint config help users? Hint: it doesn't.

@hiquest
Copy link

hiquest commented Nov 1, 2018

So is there a hack-ish way to use my own .eslintrc with a newly-created CRA project?

@russellr922
Copy link

russellr922 commented Nov 1, 2018

@hiquest you currently have two options: Eject, or:

Use react-app-rewired and put something like this in your config-overrides.js file at root of your project

module.exports = function override(config, env) {
config.module.rules.some(rule => {
    if (Array.isArray(rule.use)) {
      const eslintUse = rule.use.find(item =>
        Object.keys(item.options).find(key => key === 'useEslintrc'),
      );
      eslintOptions = eslintUse && eslintUse.options;
      if (eslintOptions) {
        eslintOptions.useEslintrc = true;
        return true;
      }
    }
  });

  return config;
}

^ haven't tested that code, but you get the idea

@patricklafrance
Copy link

@russellr86 you could also easily do it with craco, it's quite simple, all you need to do is

1- Create a craco.config.js file at the root of your project

2- Tell craco that you want to use an eslint config file

const { ESLINT_MODES } = require("@craco/craco");

module.exports = {
    eslint: {
        mode: ESLINT_MODES.file
    },
};

You can find a recipe here: https://github.com/sharegate/craco/tree/master/recipes/use-an-eslintrc-config-file

@jsphstls
Copy link

@crobinson42 that eslint console output is correct.

I'm using onClick prop on my <a> elements and the only way to appease the eslint warning jsx-a11y/anchor-is-valid is to define a valid href, ie: href="#something" and I do not want a url hash value when a user clicks the element.

The other way to fix this is to use buttons instead of links.

When create-react-app is making enforcements for the developer in their development environment, I think it should offer some configuration or disabling flag.

It's an accessibility requirement that applies to any use of HTML whether or not React is used.

@vmosyaykin
Copy link

vmosyaykin commented Nov 28, 2018

Maybe we should remove eslintConfig from package.json and only put it there on eject?
My case may be a bit strange: I've migrated an existing app to CRA (decided to do that instead of migrating to a new Webpack version manually).
I was happy to see that there is a eslintConfig option in the generated package.json.
I updated it to exclude some rules to ease migration, typed npm start just to see that my config didn't apply.
I had to look through react-scripts code and then search here in GitHub issues to see that it's the intended behavior.

@lock lock bot locked and limited conversation to collaborators Jan 9, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests