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

disable eslint #2157

Closed
maxlibin opened this issue May 15, 2017 · 27 comments
Closed

disable eslint #2157

maxlibin opened this issue May 15, 2017 · 27 comments

Comments

@maxlibin
Copy link

how can i remove eslint in npm start?

@gaearon
Copy link
Contributor

gaearon commented May 15, 2017

You can’t. Are there particular rules you want to disable? Why?

@maxlibin
Copy link
Author

Or anyway to use our own eslintrc from npm start? problem is we have our own eslint settings and its different from CRA.

@gaearon
Copy link
Contributor

gaearon commented May 15, 2017

There is a long discussion about this in #808 but TLDR is:

  • Our rules are specifically picked to not enforce style, and to only find logical mistakes.
  • We are open to making our rules work better for everyone (if you have specific ideas about adding or removing rules, please file issues!)
  • We think ESLint rules are a fundamentally flawed way to enforce coding style, and we suggest using Prettier for that instead.

We know that the community is still accustomed to ESLint styling rules (not something ESLint was built for). This means that everyone will try to use their own configs which usually include a lot of styling rules. As a net result the browser and terminal ESLint integration becomes too noisy and obscures real problems behind stylistic nits.

We are pushing the community towards replacing ESLint styling rules with tools like Prettier. Unfortunately it means inconvenience of disallowing any ESLint customization in the meantime.

In the future, when we integrate Prettier into CRA by default and enforce its usage, we might enable ESLint rule customization.

I hope this helps!

@gaearon gaearon closed this as completed May 15, 2017
@viankakrisna
Copy link
Contributor

@maxlibin If you want to enforce style with eslint, i'm quite happy with adding these scripts on my package.json and add a .eslintrc on the root of the project.

    "format": "prettier --trailing-comma es5 --single-quote --write 'src/*/*.js' 'src/*/!(node_modules)/**/*.js'",
    "lint": "npm run format && eslint --fix --config .eslintrc src"

And maybe use lint as pre commit. In my experience, fixing style nits is time consuming without added benefits. So it's better to leave it off when you actually solving the problem, and automatically format it when checked to repo.

@joshwcomeau
Copy link
Contributor

joshwcomeau commented May 22, 2017

So, I totally understand the arguments given, but we've stumbled upon a situation in which not being able to disable linting is becoming a significant headache. It's a pretty niche case, so I don't expect anything to be changed, but thought I'd mention it so that it can at least be "on the radar" of things people do.

At Khan Academy we're using Create React App for prototyping. The goal of the project is to quickly test new ideas before building them "for real" in our monolithic repo.

This project imports all of our existing modules. These files work perfectly, but fail a bunch of lint checks; specifically, no-use-before-define fails several times on almost every JSX file, because we define a style object (for use with Aphrodite) below the component that uses those styles.

The result is that the Chrome console is absolutely flooded with thousands and thousands of warnings. These warnings bury any user-specified logging, errors thrown, etc.

Anyway. As I said, I recognize that this is an unconventional situation. Leaving it here for the record, but no action or response is needed or expected :)

@gaearon
Copy link
Contributor

gaearon commented May 22, 2017

The solution to this is the same one as usual: instead of looking for a way to disable rules, file an issue and describe the specific use case where you think the rule doesn’t make sense. We are always open to relaxing rules if some of them don’t make sense for projects. 😉

In your particular example it sounds like setting variables: false option for no-use-before-define would solve the issue. I’m open to doing it.

@gaearon
Copy link
Contributor

gaearon commented May 22, 2017

To clarify, our point is not to be dogmatic. It’s to find a good subset of rules that work for everyone. Unfortunately we can’t do this if everyone makes escape hatches for themselves without reporting their scenarios. Having to go through a common process lets us create a really good ESLint config, even though it comes at the cost of having more discussion.

@gaearon
Copy link
Contributor

gaearon commented May 22, 2017

#2325

@joshwcomeau
Copy link
Contributor

The solution to this is the same one as usual: instead of looking for a way to disable rules, file an issue and describe the specific use case where you think the rule doesn’t make sense. We are always open to relaxing rules if some of them don’t make sense for projects. 😉

Right, so I didn't really explain this very well. no-use-before-define is one of the most prominent ones, but it was just an example. Other linting checks fail, and to be honest they should fail: various jsx-a11y rules, strict for unnecessary "use strict", and jsx/pascal-case, which fails because in our main repo we do some weird thing to handle i18n translations with "elements" that look like <$_>, but get pre-compiled into a translated string.

In other words, it's not that I think that the rules should be changed, because the rules do make sense (I do like the proposed change to no-use-before-define, but I certainly wouldn't want the other rules to be changed!). In our case, the issue is the legacy code we're building on top of, but there's too much of it to change, at least for now.

As I said, I recognize that this is probably not a widespread concern! I share it simply so that you can be aware that this is a thing at least 1 team is doing, but I wouldn't want create-react-app to be made more complex to handle this problem unless it became clear that a lot of users had this issue (which I doubt).

@gaearon
Copy link
Contributor

gaearon commented May 22, 2017

Yea, fair enough. In this case we optimize for the case of new code, and IMO we do the right thing by displaying them.

If the main issue is with console spam in the browser for legacy code, we can limit the number of displayed warnings in the browser (e.g. to first five files). Would that help?

@joshwcomeau
Copy link
Contributor

If the main issue is with console spam in the browser for legacy code, we can limit the number of displayed warnings in the browser (e.g. to first five files). Would that help?

Yeah, that would totally solve the issue for us.

I don't want to burden anyone else with this though, happy to submit a PR for this!

@gaearon
Copy link
Contributor

gaearon commented May 22, 2017

Added in #2327.

@gaearon
Copy link
Contributor

gaearon commented May 22, 2017

Both changes should be out in react-scripts@1.0.5.

@joshwcomeau
Copy link
Contributor

Both changes should be out in react-scripts@1.0.5.

Yay, thanks Dan! :D

@oelmekki
Copy link

oelmekki commented Jun 29, 2017

I have this method:

  currentPage() {
    switch ( this.state.action ) {
      case "projects":
        if ( this.state.projects ) return <Projects items={this.state.projects.items} />;
        break;

      case "builds":
        if ( this.state.projects ) return <Builds project={this.state.projects.currentProject} />;
        break;
    }

    return <NotFound />;
  }

Which basically enforces a condition on each matched case and fallback to a <NotFound /> component if app is not properly setup or if the required action does not exist.

This, of course, triggers this lint error:

Line 26:  Expected a default case  default-case

Well, no, I won't duplicate the line returning <NotFound /> in a default case, since I need it anyway when condition in matched cases fail. I think this illustrates why linters are so annoying : wanting to prevent common errors, they force us to avoid perfectly legit, although unusual, code.

I won't ask you to change create-react-app because of this, when I'll have enough of this warning, I'll just eject and remove the linter, but I had to let you know since you say you want to detect only logical mistakes and not styles.

By the way, if we disregard coding styles, is there anything a linter can do that flowtype can't?

@gaearon
Copy link
Contributor

gaearon commented Jun 29, 2017

I understand it is subjective, but in this case I think the warning is valuable. It is easy to make a mistake in code that relies on both switch, if, and returns in different places for control flow. Maybe you won’t make mistakes with this, but many people will.

Generally when I write code like this, I think how to restructure it for clearer and more explicit control flow. I know I’m not smart enough to come back to it after a few months and not to accidentally misunderstand it. Again, this is debatable, and your point is valid, but I do believe having this warning is a net win. If it’s common enough for you to eject, and you don’t agree it’s worth rewriting the code, well, let’s agree to disagree 😛 .

Well, no, I won't duplicate the line returning in a default case, since I need it anyway when condition in matched cases fail

You don’t have to duplicate it. You can intentionally fall through.

default:
  // Fall through
  break;

This is enough to appease the linter. This tells the person reading your code (or you in a few months) that lack of return there was intentional, and not an accidental omission.

@oelmekki
Copy link

Sure, there are tons of way I could refactor it, or even totally remove the method ("hey, why not use react-router instead?"), but refactoring legit code to satisfy linter always makes me roll eyes (and this is about as much as I'm annoyed - not a big deal). I just wanted to point out this was a case breaking the "not enforcing coding style" rule.

So, I guess we mainly disagree on what is coding style and what is not :) That's ok, though, just wanted you to know, but I can live with it. Create-react-app rocks anyway, keep on the good work!

@gaearon
Copy link
Contributor

gaearon commented Jun 29, 2017

Thanks!

So, I guess we mainly disagree on what is coding style and what is not

I agree it's a thin line. Generally we err on the side of enforcing that potentially ambiguous code is rewritten more explicitly. Adding a default case is generally valuable because it forces the person writing the code to think "what happens if someone adds another option/route/query". And thus prevents timebomb code that blows up when somebody extends it.

@oelmekki
Copy link

I get you, this is a library, used by many, so you have to consider which errors will be the most common. In this case of "enforcing default: even with a fallback after the switch", I guess we can say it's a logical mistake consideration with a coding style side effect :)

@Andsbf
Copy link

Andsbf commented Aug 1, 2017

If i can't disable or tweak lint, how can I get around this?

Failed to compile.

./src/~/react-router-dom/es/index.js
  Line 3:   Import in body of module; reorder to top  import/first
  Line 5:   Import in body of module; reorder to top  import/first
  Line 7:   Import in body of module; reorder to top  import/first
  Line 9:   Import in body of module; reorder to top  import/first
  Line 11:  Import in body of module; reorder to top  import/first
  Line 13:  Import in body of module; reorder to top  import/first
  Line 15:  Import in body of module; reorder to top  import/first
  Line 17:  Import in body of module; reorder to top  import/first
  Line 19:  Import in body of module; reorder to top  import/first
  Line 21:  Import in body of module; reorder to top  import/first
  Line 23:  Import in body of module; reorder to top  import/first
  Line 25:  Import in body of module; reorder to top  import/first

src/node_modules/react-router-dom/es/index.js

import _BrowserRouter from './BrowserRouter';
export { _BrowserRouter as BrowserRouter };
import _HashRouter from './HashRouter';
export { _HashRouter as HashRouter };
import _Link from './Link';
export { _Link as Link };
import _MemoryRouter from './MemoryRouter';
export { _MemoryRouter as MemoryRouter };
import _NavLink from './NavLink';
export { _NavLink as NavLink };
import _Prompt from './Prompt';
export { _Prompt as Prompt };
import _Redirect from './Redirect';
export { _Redirect as Redirect };
import _Route from './Route';
export { _Route as Route };
import _Router from './Router';
export { _Router as Router };
import _StaticRouter from './StaticRouter';
export { _StaticRouter as StaticRouter };
import _Switch from './Switch';
export { _Switch as Switch };
import _matchPath from './matchPath';
export { _matchPath as matchPath };
import _withRouter from './withRouter';
export { _withRouter as withRouter };

@viankakrisna
Copy link
Contributor

viankakrisna commented Aug 1, 2017

can you move the node_modules folder outside of src? It should be placed in the root (beside ./src) for dependencies. All files inside ./src should only contain user created code.

@Andsbf
Copy link

Andsbf commented Aug 1, 2017

thanks @viankakrisna , it was by accident I added a package to yarn and created a new package.json inside src

@YagoLopez
Copy link

For anyone interested, in my environment (react+typescript) I can prevent tslint throwing messages in console just deleting all tslint.json rules and leaving an empty object {}

@ghost
Copy link

ghost commented May 6, 2018

Is it possible to disable eslint during npm start and only run it after a commit, or before a build it done?
My goal is to increase development time.

@ChristosChristofidis
Copy link

If you are ok with ejecting you can disable it directly from webpack config I think

@Kleptine
Copy link

For my specific project ESLint ends up taking about 10 seconds. After ejecting and disabling the build goes down to a second. :(

I really don't want ESLint, I have my own linting systems. :|

@flyingcrp
Copy link

oh...in CRA 2.1.1 how can i overwirte the esling rule .? i have own linting config. maybe i have ocd . :(

@lock lock bot locked and limited conversation to collaborators Jan 8, 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

10 participants