customize eslint rules (.eslintrc) #808
Comments
|
@julesbou You may need to update to a newer release (I believe 0.5.0), but in general the following has worked for me for tweaking/extending eslint rules:
(eslint-config-react-app, eslint, babel-eslint, eslint-plugin-react, eslint-plugin-import, eslint-plugin-jsx-a11y, and eslint-plugin-flowtype) |
|
@moosehawk Does this work in the browser's console too? It makes sense to work with Sublime/Atom etc but I think (not 100% sure) that Webpack is still logging eslint warnings using the default config provided by |
|
@manosim I just checked and it looks like you're right. It does not log the extended eslint output to the browser with the current setup. |
|
Why not changing useEslintrc to true? This would allow to use an existing eslintrc and the default config as fallback if no eslintrc exists. |
|
@bjoernricks does changing this setting to true will work ? |
|
I don't think it would be a good solution. We aggressively show lint violations (in browser for errors, in console for warnings), and so we didn't include any style rules in the config. I think style rules should be handled completely separately, before you commit. They shouldn't distract you during development or be loud in browser or terminal. So I suggest using tools like standard and their autofix functionality instead of trying to add more rules to Create React App configs. Read more in #734. |
|
@gaearon so, you just suggest to close this issue and to fix eslint warnings by running a tool, is it correct? What if we have different tastes, while I agree with most eslint rules proposed by CRA, there's one or two I would like to remove, for example:
Of course I could send a PR to change these rules, but then I'm not sure they would get merged as some developers might have different tastes (again). Why not make everyone happy and allow .eslintrc files (which is a pretty standard things by the way). |
I get your point but some people (including myself) are used to enable warning for style rules during development. It's not something we should add to create-react-app by default, I just think it could be nice to have this possibility. And if we extends the react-app eslint conf with this: before adding custom rules, we shouldn't have any conflicts when react-scripts updates. |
Create React App config is not about tastes. We tried to only pick rules that usually indicate errors in the program. If you find rules you want to disable, send PRs and let's discuss. |
Even if you had a |
Configuring webpack manually was also pretty standard. I think demand for this project shows that not all "standard" things are great for everyone and sometimes we need to rethink approaches 😉 . |
For me it does work. Just included and did override guard-for-in rule. |
|
As I said, if you think some rules shouldn't be in default setup, please send PRs. |
|
@gaearon Configuring webpack was awful and it's one of the reasons we have seen so much boilerplates rising. And it's also why create-react-app is really appreciated when working with react. I understand the philosophy behind CRA is to not expose any configuration and have a working project out-of-the-box. The .eslintrc is the only thing which is missing for me, and it's not a feature, it's just about style rules. |
|
Ok I think I got it. @gaearon you would like to apply only error checking in dev server. In that case the create-react-app eslint config should be as minimal as possible and should not enforce any taste of style. Then it would be possible to provide an own eslintrc with additional style enforcing rules for usage e.g. in vim. |
|
Yep, exactly. |
|
@tseho Are any encapsulated style enforcement tools like "standard" working for you? Why not? |
|
@gaearon Currently, our rules does not match the ones in standard (and if it was the case, it would be much easier) For now, I installed eslint in my devDependencies and I use a separate script which is loading our .eslintrc. |
|
salut @tseho; mind sharing your script to load .eslintrc? |
|
@julesbou I added a "lint" entry in the scripts of package.json with something like This was the fastest solution I could come with, without modifying CRA configuration. But this is far from perfect, both my eslint and CRA eslint are running at the same time with two differents configurations. |
|
@tseho thanks. I just did the same. Maybe CRA should add a way to disable eslint, so we can replace it with our own |
|
@gaearon use case for rule customization: I am using flow, and i want to be told when I forget to put a Its something that costs me time, something that 100% should be called out in a dramatic way during development, but not something which should be enabled by default. |
|
I’m a bit confused. As far as I can tell this rule is already enabled. |
|
It isn't actually catching anything. I think by default it catches typos but not detecting presence?
There are a few types of flow codebases. If you are migrating to flow, it may only be enabled in certain spots. If you are aggressively checking, you want it everywhere, and missing it in a key spot may result in large numbers of errors being unchecked. Setting that rule to "always" would probably result in different people complaining. Maybe this is something that should be handled by flow itself rather then relying on eslint? So far I have found the constraints in CRAs choices of settings force me to question if I really should be doing the thing I would normally just configure without a second thought. Every time so far, I have chosen against ejecting, which is great and a positive exercise :). This would be the first time time where I really thought something should be an option, so came here to open an issue. |
|
Hmm I see what you mean. Can we add/write a different rule that only asks you to add |
|
I believe that is the default behavior of that rule. The problem is that default is great for certain codebases, and for others its a very big problem. On a 2kloc project, I forgot to add the annotation on a few index.js files. Realized my mistake when type checking wasnt working somewhere, I added it, and ended up with 55 errors, some of them actually pointed me to design flaws. I would have loved to catch them the first time through, so I went looking for how to avoid this issue in the future. Turns out eslint is "the way", but I use CRA. As a second data point, chatting about this with a friend on slack, he was under the impression he was fully type checked on a larger project. Added the eslint rule, and then sent me this message
I think that establishes a case that this is a good thing to have on as "always" on any flow project where full type checking is expected. But flow has use cases for partially checked projects, which is why I believe they went with a whitelist approach in the first place. So I'm pretty sure if you configured that rule to be more aggressive, other folks would complain. I think an argument could be made that enforcing the presence of |
|
I found a way to hack https://gist.github.com/cncolder/19354ba59146c55071841f6fa274da66 |
|
@gaearon what if create-react-app detects if there is a You could still use the default rules by using |
|
@DanielHoffmann That's not CANNOT. Only DO WANNA. So now use custom-react-scripts or hack it by yourself. |
|
We're currently using create-react-app for a university project with people from It's kind of hard to explain to them how setup stuff on their os/editor and it get's even worse in regards to code style as everyone has a different background. |
|
@sakulstra yes I agree with you. Customizing eslint rules is probably the first thing someone might want to customize in a project. The way I proposed before of picking up .eslintrc from the root of the project would not add much complexity to the configuration and I believe this should be an exception to the "zero configuration" principle of create-react-app. After all the way I proposed would not change anything if you do not have an .eslintrc file. I believe in this case we have to be pragmatic, customizing eslint rules complexity is nothing like adding LESS or SASS to the project. I don't think anyone would eject or fork create-react-app just to customize eslint rules, but would just live with bad eslinting. That is what I am doing at least, I rather have bad eslint rules than eject or fork create-react-app. I am still willing to do a PR on this if the maintainers approve. |
|
@DanielHoffmann @sakulstra implementation in fork is the same. If you add a .eslintrc then it overrides |
|
I've been reading threads about this all afternoon, but I apologize if I've missed an existing answer. I'm working on an app that was built with CRA. Awhile ago we added The solution we're looking for would allow us to do the following;
Solutions that aren't satisfying:
A solution that would work: Being able to define a root level We're open to other solutions and ideas as well. @gaearon do you have a strong position on this issue, or is it something that the CRA maintainers are still debating and open to various approaches? |
|
I just don't think style linting rules are a good approach. It's meaningless to spend developer time on doing something a machine can do. I would just suggest waiting for https://github.com/jlongster/prettier (or similar tools if you don't agree with its choices) and adopting it. |
|
Have you considered maintaining a fork? The recent patch to backstroke should make this super easy, especially if you pair it with something like https://github.com/atlassian/lerna-semantic-release or https://github.com/semantic-release/semantic-release. |
|
I like Prettier. It would be awesome to have something like as an editor plugin, especially if it could read rules from an eslint rule set that matches a team's rule preferences. But its more of a developer convenience tool than a tool to enforce style standards in an app. I get that CRA doesn't want to have more than basic linting out of the box, that makes sense. But it would be nice to be able to take advantage of the linting tools built in to CRA (automatic terminal, console, and viewport reporting) for a custom rule set rather than having to add a completely secondary toolset. Maintaining a fork of CRA is something that our team just doesn't have time for. We're already maintaining forks of dozens of other projects. |
Not sure I understand why? You can run Prettier (or alternatives) on all changed files as a commit hook. The CLI isn't quite friendly yet for this use case but they intend to focus on this use case after it's more stable. cc @vjeux
I don’t agree that it would be valuable to spend the screen space on bad indentation or a missing space.
As @Timer mentioned above, you might want to give backstroke bot a try—it maintains the fork mostly automatically. I don't think you'd get a lot of merge conflicts if you just replace the ESLint config. |
|
I can run a lint script with a commit hook as well. The problem is that developers are seeing one set of warnings in the Terminal/Browser during development, and a different set of warnings on the commit hook. The linting that they get in their editor can only be one set or the other, and they have to choose between those sets and manually configure them. Trying to mentally keep track of why there are two separate linting rulesets and remember which one should be used is something that developers have a hard time doing while focusing on other things. |
|
To be more concise; the problem is having two separate sets of linting rules in the project. It makes our standards unclear when there are two "sources of truth" for our linting rules. |
This is not what Prettier does. It doesn't show any warnings. Prettier is reformatting code automatically (check out its README, blog post, and the demo). My point is it's not very useful to spend developer time on fixing style nits when a tool can do that. I think that at some point, if it proves stable enough, we might ship it by default with CRA. |
Wouldn't it make sense to remove eslint from CRA completely then? Developers getting mixed messages about which linting rules to adhere to is confusing for our team, and right now there is no solution to that except just using the default CRA linting rules. |
No, why? I think ESLint is very useful for finding actual mistakes in the code. This is what our current config is focused on. I don't think, however, that it's useful for enforcing style. Instead, I think that an automatic formatter is the best solution to this problem. We currently don't provide any integration with one, but encourage you to try Prettier (and report any bugs in it to help get it to a stable point). We will likely adopt Prettier at some point in the future. Does this help? |
|
I agree with your point that enforcing style is a waste of developer time. I think adopting something like Prettier into CRA would be a great thing to add to the roadmap. But it doesn't solve the present issue of having duplicate sets of linting rules applied to our project when we want to apply our own. For now we'll just create a duplicate set of linting rules, have our developers reconfigure their editors to use that set of linting rules, modify our start script to be |
Arguably one can see “wanting to apply own own“ as part of the problem 🙃. The premise of tools like Prettier is that you just embrace their way of formatting and don't impose any more style rules. If you're not comfortable with the idea of doing this, I unfortunately don't have a better solution for you. While Prettier isn't quite ready yet, I'm confident this is the direction we are heading in, and I wouldn't like to introduce an escape hatch now if we're going to promote automatic formatting later anyway. Now, if you need custom lint rules to check against actual mistakes in the code, by all means, please propose to include them into our default setup. We are happy to evaluate such requests. I'm sorry if this is not very helpful in your situation. This project has a specific vision, and never worrying about style nits aligns with it very well. I'm not confident that, for example, supporting company-wide style guides aligns with it because once you're at this point, you might as well maintain a fork for your company, like many people do. I also know it's hard for new approaches to get traction when challenging established practices. I'm willing to take some flak for pushing projects like Prettier and getting more people comfortable with outsourcing their code style to the machine. My anecdotal evidence says that even people who initially oppose the idea really appreciate the change after living with it, and I want to help push more people try it, even at the cost of some short term inconvenience. |
|
Apparently Bitbucket (among others?) has started setting In a case of Bitbucket, the environment variable cannot be unset and changing the value does not affect either. Finally |
|
This is intentional, as people previously asked for ESLint failures to crash CIs.
Unused variables are often indicative of bugs. We generally try to only use lint warnings for real issues that serve as proxy for bugs, and this is one of them. This is another reason we don’t have any style rules, as those don’t really affect correctness. |
|
While recognizing @gaearon's view that CRA should only lint against things that are very likely to be errors, rather than style, I find myself disagreeing that CRA's current config is doing that. Maybe that means I should be making pull requests, but having run into two of these things in just the first couple days of using CRA, I suspect the problem may be disagreement about whether these rules are about style or about likely errors. Here are the rules I've run into so far, that I think are clearly wrong from the standard of "only trigger in response to a likely error". First, three warnings related to the use of a labeled for-loop. This is clearly about style. I didn't introduce a label by mistake! I added it because it is the "best" way for me to continue an outer loop from inside an inner loop. I put "best" in quotes because you may disagree, and advocate some use of flags, exceptions, or functional techniques. But I disagree in the code I wrote, and that's style enforcement. It's clearly false that introducing a label is a sign of a bug. Second problem. I have mutually recursive classes. That seems common enough. In my case, a Game class and a Team class. Each refers to the other class by name (they need to be able to construct one another). I get several This code is perfectly fine, and the alternative is something crazy, IMO. (Some discussion here, btw: http://stackoverflow.com/questions/37144786/es6-early-use-of-classes) I guess my point is that a) These rules ought to go, and b) If you disagree (and you very well might) then the stance that CRA's eslintrc is all about style is not going to be a "bright line" with clear answers. And that argues for easier local addition or subtraction of rules, without ejecting. |
|
Labels are generally used by advanced programmers and introduce otherwise unnecessary complexity.
If you must make your code more difficult to understand and not resort to functions, you can safely ignore the warning using the directive suggested in the dev output. We understand that you may feel strongly about this, and are more than happy to continue the conversation and potentially disable the rules. |
|
I guess the complexity of debating these rules is exactly my point. The stated goal is to have rules that apply only to things that are mistakes, not style. My claim, bolstered by your reaction, is that such a bright line does not exist. So my real point is about this very issue: being able to change eslint rules is important and can't be wished away by saying, "We'll only have rules that definitely apply to only truly bad code." PS. Since functions also cannot perform a non-local return (that is, out of two "layers" of looping forEach()'s or similar) without using exceptions, they do not help. But PLEASE, my point is that this is the rathole that claims about having rules that only apply to mistakes tries to take us down. Let's not go down it. Let's address this issue, and have some reasonably convenient way to configure the linter (cli and browser output, hopefully). |
|
As I said before, please file issues (or send PRs) for individual cases you disagree with. I’ll be happy to review them. I agree with you on some of these points and consider relaxing the config a little but it’s hard to discuss this when you propose a batch of mutually unrelated changes on a long thread 😉 . |
It does exist. In fact I just merged a PR relaxing a lint rule a few days ago: #1773. I think you might have missed @Timer’s point—it’s not that we disagree, but it’s that we ask you to file a new issue to discuss a specific case:
Let’s just do that, and punt on discussing a broader philosophical issue for now 😉 . I’m going to lock this issue because of this. To sum up for future readers:
|
I've been using CRA for a few weeks and since the beginning i'm looking for a way to tweak eslint rules.
Do you have any plan to allow (easy) customization of eslint rules?
The text was updated successfully, but these errors were encountered: