Enforce a consistent coding style by adding a linter

[jankeromnes]

trafficstars

In order to guarantee a consistent coding style, and prevent some types of coding errors to enter the code base, we should:

• [x] Automatically lint the server code (Node.js)
• [x] Automatically lint the website code (ES5.1 JS)
  • [ ] Handle every error argument, and every possible promise rejection
  • [ ] Use const everywhere possible, or let otherwise (only in ES6/server code)
  • [ ] Force null, 2 for every JSON.stringify call
  • [ ] Forbid empty lines between a function comment (e.g. // Do stuff.) and function declaratation (e.g. function doStuff (thing) { or exports.doStuff = function (thing) {)
  • [ ] Force an empty line below every if/else and try/catch block
• [ ] Automatically lint the HTML in /static/
• [ ] Automatically lint the CSS in /static/

Here are a few guidelines that I think should be respected:

• We should write very short but very simple code (no unreasonably convoluted code and no black magic)
• We should use explicit, full variable names (e.g. not c but chunk, not req but request, not values but durations etc.)
• We should sort most things alphabetically (requires, variables, parameters, HTML attributes, etc) unless it makes things more confusing

Here are a few tools we could use:

• eslint: A great JS linter, similar to JSLint and JSHint (we would need to write a coding style configuration for Janitor, or auto-detect it from our code)
• standard: Basically ESLint with powerful, pre-configured rules and no customization possible (it is opinionated but powerful and convenient. Maybe the most controversial rule it enforces is "no semicolons")
• flow: A static JS type checker made by Facebook.

I would be OK to enforce standard for Janitor, even if it removes all semicolons. @bnjbvr @nt1m @Coder206 @notriddle or @espadrine, do you have any thoughts or preferences about linting our code?

[bnjbvr]

Just wanted to add prettier to the list of possibilities: it just does styling code, no linting nor static analysis, but the JS rendering is great with minimal configuration (the most controversial choices can be configured, such as semicolons or not, etc.). You can set it, and forget it: with a pre-commit hook + CI jobs that check that the output of prettier is the same as the current state of the code.

standard feels like the worst option to me: not configurable and it does remove all the semicolons. It's a barrier to entry (new JS programmers might not know all the rules of ASI), it makes reading code harder (have to do recurrent back-and-forth between end of line and next line to understand if there could be some interactions) and it's just plain ugly in my opinion (personal preferences 🤷‍).

So prettier plus something like eslint + predefined rules (I heard airbnb's ones are very reasonable) would be my first choice.

[nt1m]

I like the DevTools eslint rules: https://dxr.mozilla.org/mozilla-central/source/devtools/.eslintrc.js

[espadrine]

I use eslint on shields.io, so I am obviously biased, but it is objectively the best human achievement since speech.

standard feels like the worst option to me: not configurable and it does remove all the semicolons.

:+1: (there's also a semicolon version but it defeats the name)

[jankeromnes]

I guess the consensus looks like using eslint with our own configuration (which can be derived from standard but with semicolons, or auto-detected from our sources, or hand-crafted).

[magopian]

FYI the devtools profiler started using prettier lately: https://github.com/devtools-html/perf.html/pull/402

There's everything in there, including git hooks.

I would be happy to mirror that for janitor if you're ok with that.

[nt1m]

I've added some items to address in the issue