Feathers: Consider a more powerful linter

[mmwtsn]

As the Feathers project grows having a linter that can better enforce some JavaScript best practices would help to keep the code base consistent and free of easy-to-catch errors.

I've been using ESLint at work for about a year now and I think it has done a lot to improve the quality of our code base as it has grown over time. There are tens if not hundreds of rules to chose from that can enforce everything from correctly defaulting to const, ensuring errors are handled in callbacks, and even validating JSDoc documentation comments. JSHint is a good linter but there is a lot that it misses.

Rather than roll your own set of rules I'd recommend pulling in a default configuration that has already been released and used by the community such as Airbnb's or semistandard. This would allow for the Feathers lint configuration to be distributed as a standalone module which makes it much easier to update across the ecosystem than managing lint configs individually in each repository.

[ekryski]

@mmwtsn definitely. As part of the new CLI work we'll be offering the ability for you to choose your linter, just like Vue's CLI tool. Going to close this in favour of tracking it in https://github.com/feathersjs/generator-feathers/issues/112. Thanks for bringing it up!

[mmwtsn]

@ekryski sounds good!

I would love to see a single code style picked for the Feathers community. There are certainly a variety of different code styles that are all valid but having each Feathers module be written in a consistent style helps maintainers and contributors alike. I think the Go team said it well when discussing their thinking behind go fmt. It's just one less thing to think about.

Leaving the choice of database adapters or websocket modules up to the community makes sense but I'm not sure there's much value in, say, having some modules written with semicolons and some without.

[ekryski]

💯

[daffl]

I think @ekryski was talking about code styles for the generated application.

The code of all officially supported modules should already follow the same code style and JSHint rules that catch actual errors and higher risk warnings. I'm not sure how worthwhile it would be to make them any stricter. My experience is that too picky code guidelines in open source projects can unfortunately even be counter productive by raising the barrier of entry to contributing. I'd rather fix up a contribution in a different code style that has tests (which is the bigger challenge really) than have someone not submit it at all because they are getting tons of validation errors.

[ekryski]

Indeed I did mean the linter for the generated code, so this isn't the same but is somewhat related, so I'll re-open.

Re-reading, I also agree with @daffl. We want to make contributing easier as opposed to harder and the core team is responsible for:

• a) making a judgement or suggestions on when not to merge something (the default is merge)
• b) fixing up any code before a release so that it follows a similar style or asking that the style be changed if it is significant.

With that said, I would be in favour of using ESLint, making it use a fairly standard spec, and also having it as a standalone module.

Splitting it out will reduce the overhead of maintaining it in the long run, and if it's a fairly well adopted spec like Airbnb's it shouldn't be too much of a barrier. In general, I think it's also a good way to educate people on what their code should probably look like. In my experience, consistency is the most important quality in a code base, especially as people naturally come and go. It's a tricky balance.

Thoughts @feathersjs/core-team?

[daffl]

Maybe we can adopt a policy to keep issues like this open for another 2 months to see if anybody is willing to pick it up and update every module accordingly. I probably won't since there are a lot of higher priority things in the foreseeable future and I am ok with the way it is set up right now.

If nothing happens we will close this issue in two months.

[ekryski]

@daffl 💯 we get a lot of awesome suggestions but we have much higher impact stuff to work on as a core team and limited resources. I think 2 months is great time frame. So we'd love the help from anyone who is willing to update modules to a different linter.

[radiovisual]

So we'd love the help from anyone who is willing to update modules to a different linter.

Which linters/configurations would be considered as the preferable choice for the modules? I personally use XO, which is simply an opinionated ESLint configuration that is super easy to "plug n' play", but the other options listed by other users in this thread are good too -- so with a bunch of linters and an infinite number of possible linter configurations...how would you want to settle on one?

[daffl]

I'm not sure we can. There are really two Linter discussions going on here I think:

1) The code style and Linter used by Feathers projects (core and plugins - pretty much any code in the https://github.com/feathersjs/ organization). As I said before, I'm ok with the way it is set up at the moment and unless @ekryski or @marshallswain (the other two people reviewing and releasing code submissions at the moment) strongly disagree I think everything is set up in a way that is manageable and does not need to be changed.
2) The application code style in a generated application. We are working on a new generator and CLI and our thoughts were to possibly let you choose what Linter you want to set up but not generate the code accordingly. Normally it should only be a few lines of code to change to get the Linter passing and having to maintain different templates for different Linter configurations (or putting conditionals everywhere) is something I'm very reluctant to do. I thought I saw a tool that allows you to format code files according to your Linter settings but I forgot where. If it can be automated, it might be something worth looking into.

[radiovisual]

Ok, I understand. Thanks.

I thought I saw a tool that allows you to format code files according to your Linter settings but I forgot where

You can auto-lint your project files with ESLint. ESLint has an auto-fix option where you can tell it to fix your linting issues automatically. This option is also extendable to your own custom rules, where you can decide what happens when something gets "fixed".

Pretty cool.

[ekryski]

Yup @daffl is bang on! I've basically set up the generator a loose ESLint config (based off of ESLint Standard) that both ESLint and JSHint will pass with the generated code. So you'll be able to choose whether you want to use ESLint or JSHint with a generated app. From there you can tweak the ESLint config (and therefore make modifications to the code) to support whatever style you want.

This strikes a good balance between offering flexibility in linter selection while preventing us from having to conditionally generate code based on the linter chosen (which is quite complex and error prone).

[ekryski]

@radiovisual that's good to know. I don't think we'll do that as part of the generator as it might cause false errors or confusion if done "automagically" (especially if it doesn't catch everything). I'd like to leave that up to the developer to explicitly run in they would like.

Thanks for the input everyone! I think we can close this. Please watch for the CLI v2.0. Should be landing the next week!

[lock[bot]]

This issue has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue with a link to this issue for related bugs.