Move to ESLint?

[callumlocke]

I've been using ESLint recently instead of JSHint, and it's just so much better in every way. Much more flexible and configurable, and much better documented (configuration guide, list of rules) .

In particular JSHint is behind when it comes to ES6/7 stuff. The Babel community has embraced ESLint for this reason. (ESLint lets you plug in a different parser, so you can use babel-eslint which means ESLint can understand anything Babel can compile.)

A good .eslintrc for Origami components (if we're using import etc.) might be something like this:

parser: 'babel-eslint'
ecmaFeatures:
  modules: true
env:
  es6: true
  browser: true
rules:
  guard-for-in: 2
  curly: [2, 'multi-line']
  # etc...

[AlbertoElias]

I agree, I think it's important to move if we're going to use ES6 more and more

[callumlocke]

I'm trying to put together an .eslintrc based roughly on the recommended Origami .jshintrc, which is this:

 {
    "eqeqeq": true,
    "forin": true,
    "freeze": true,
    "immed": true,
    "newcap": true,
    "noarg": true,
    "nonbsp": true,
    "undef": true,
    "unused": "vars",
    "strict": true,
    "trailing": true,

    "esnext": true,
    "expr": true,
    "gcl": true,
    "globalstrict": true,
    "lastsemic": true,
    "multistr": true,
    "smarttabs": true,
    "sub": true,

    "browser": true,
    "nonstandard": true,
    "predef": ["require", "module", "exports", "requireText"]
}

two questions

1. JSHint options trailing, gcl, smarttabs are apparently not documented and I don't know what they're supposed to do?
2. ESLint rules are not just on/off, they are level 0, 1 or 2 (off, warning, error). Would we want everything to be a hard error?

[AlbertoElias]

I was going to work on this tomorrow and add it to OBT. Thanks a lot for the help. Currently JSHint just errors, so I'd say so. If you think some shouldn't be a hard error, please mention them and we can discuss it.

gcl is related to the Google Closure Compiler, so that's not necessary anymore. I think trailing is related to trailing whitespace, and smarttabs is related to using tabs, which lintspaces should be doing, so I wouldn't worry about those

[callumlocke]

cool let me know if you need any help, I've been using eslint for a while

[AlbertoElias]

I'm going to suggest the following config file. Let me know what you think. I've tried to move over the rules from JSHint one by one, also added a few that I believe would follow the spec

{
    "ecmaFeatures": {
        "modules": true
    },
    "env": {
        "es6": true,
        "browser": true
    },
    "rules": {
        "eqeqeq": 2,
        "guard-for-in": 2,
        "no-extend-native": 2,
        "wrap-iife": 2,
        "new-cap": 2,
        "no-caller": 2,
        "no-irregular-whitespace": 1,
        "no-trailing-spaces": 2,
        "no-multri-str": 0,
        "dot-notation": 0,
        "strict": [2, "global"],
        "valid-jsdoc": 1,
        "no-loop-func": 2,
        "no-multi-spaces": 2,
        "no-multiple-empty-lines": 2,
        "one-var": [2, "never"],
        "constructor-super": 2,
        "no-this-before-super": 2,
        "no-var": 2,
        "prefer-const": 1
    },
    "globals": {
        "require": true,
        "module": true,
        "exports": true,
        "requireText": true
    }
}

[charypar]

/cc @imranolas who's looking at getting eslint into obt at the moment as well

[callumlocke]

just read the docs for all these rules, looks good! a few things...

• I don't think you should have the strict rule, because "use strict"; has no effect in ES6 modules (it's already on by default, and Babel adds it when compiling down to ES5).
• IMO no-loop-func is no longer a good idea, it's just a redundant restriction now that you can have block-scoped variables with let and const. For example this code is the reason why functions in loops are considered bad (it just prints '10' ten times). But if you use block-scoped variables instead, it works nicely (and Babel's method of making this work is surprisingly nice and simple).
  • Although you can't prevent people doing it wrong by using var in a loop... unless you also enforce no-var, which I would advocate because in my opinion there is no good reason to use var any more.
• no-multiple-empty-lines and no-multi-spaces – personally I find these rules a bit too blunt. I often use two blank lines, and sometimes extra spaces to align object values, not out of sloppiness but for readability.

everything else looks good to me, there are a few rules I hadn't heard of but look handy

[AlbertoElias]

1. Not all code will be ES6 modules, so I would enforce it. Don't like trusting Babel to add it to be a brilliant idea, I'd rather be explicit about it.
2. I believe the problem with functions in loops is performance, not people using it wrong cc/ @samgiles
3. We are enforcing no-var
4. Products don't need to follow these rules, you can run verify with your custom config file. For us, it's useful that all components follow a similar style, and we decided to go with spacing in that way

[callumlocke]

1. trusting Babel to add use strict when compiling to ES5 is the same as trusting Babel to convert ES6 arrows into bound ES5 functions - it's just part of compiling from ES6 to ES5. in fact I don't think ESLint will even observe strict: [2, 'global'] if you've enabled es6 and modules. It's probably impossible to make a single eslintrc to cover both ES5 and ES6 cases perfectly. Maybe there should be two configs?
2. no-loop-func is intended to avoid scope confusion. It's true there actually can be a very slight perf benefit to moving a function outside a loop, but I think that's an optimisation vs readability decision for a human, not something for a linter to enforce unilaterally. (for comparison a plain for-loop is always much faster than forEach, but we wouldn't use a linter to ban forEach.)
3. cool, I missed that
4. fair enough

[samgiles]

On 2. My personal opinion is that we shouldn't enforce this, I can see legitimate reasons to create a closure in a loop.

[AlbertoElias]

If it ignores it in that case, it makes it valid for both cases. Ok, let's remove the no-loop-func rule

[callumlocke]

I guess it does!

[expe]

We should add no-const-assign, reassigning variables declared as const will cause runtime errors so I think it would be nice to know that it's problematic before running the code.

[AlbertoElias]

+1, I'll add it

[callumlocke]

I thought rules that prevent runtime errors (like no-const-assign), as opposed to just matters of style, would be enabled by default. Then I realised the docs aren't very clear on this so I asked about it. It turns out:

• no rules are on by default
• but some rules are 'recommended' (and these are labelled "(recommended)" on the main rules listing)
• you can opt-into this recommended ruleset using extends: 'eslint:recommended'
• you can also extend other specific files, e.g. within node_modules, and you can keep extending indefinitely.

So do you think it might be good to have a couple of base files in the origami-build-tools repo which individual projects can extend, instead of having every project contain the full list of rules?

[AlbertoElias]

That is an interesting feature, but for our use case, this file is really a set of rules that modules MUST follow. They're also a decent set of rules that other products have decided to follow, and for that use case, the extending functionality is interesting. But it is a linting file for components. Products can pass in their own file to eslint if they wish to, and they can extend our config

[callumlocke]

For example:

In the origami-build-tools repo:

origami-build-tools/.eslintrc-base:

extends: 'eslint:recommended'
rules: [
  # (plus any of our own overrides)
]

origami-build-tools/.eslintrc-browser:

extends: './eslintrc-base'
env: [
   'es6',
   'browser'
]
rules: [
  # (any further browser-specific overrides that we want to add)
]

origami-build-tools/.eslintrc-node:

extends: './eslintrc-base'
env: [
   'es6',
   'node'
]
rules: [
  # (any further Node-specific rules that we want to add)
]

In projects scaffolded using obt init:

.eslintrc:

extends: [
  './origami-build-tools/.eslintrc-node',
  './origami-build-tools/.eslintrc-browser'
]
# Add any project-specific overrides here

Or maybe obt init could set up the project directory structure so that all clientside JS goes in one directory with its own .eslintrc, and all serverside JS goes in a different dir with its own .eslintrc, and each of those extends the relevant base config in origami-build-tools.

The advantage of extending is we can just maintain the base config(s) in one place, origami-build-tools, which is a semver-controlled dependency of every project anyway.

[callumlocke]

this file is really a set of rules that modules MUST follow.

I don't follow... isn't that a good case of extending, then? You could mandate that every component simply extends origami-build-tools/.eslintrc-origami-browser and leaves it at that (without any overrides). Seems less error prone than having each component contain its own copy of the central Origami config, which might get out of date.

[AlbertoElias]

The thing is, no component maintain a copy. They run obt verify, and obt uses its config file that corresponds to all components. So it's already centralised

[callumlocke]

Oh I see. One possible problem, if there's no .eslintrc file in a project directory, you can't run the command $ eslint as you'll get different results than with $ obt verify. So you won't be able to use any third party tool that expects the $ eslint command to work as documented, like SublimeLinter-eslint, atom-linter or eslint-nibble.

If this isn't a problem for component authors then don't worry.

[AlbertoElias]

I know, but we expect module developers to be using obt, so it's not much of an issue and no one has complained really. And regarding plugins, I've seen people copy are file to their home directory to solve that. Keeping in mind we hardly ever update linting rules, it works

[triblondon]

These rules to be updated in the spec once OBT and the build service are next released, which will incorporate webpack and eslint.

[AlbertoElias]

Currently, the almost definite set of rules seems to be:

{
    "ecmaFeatures": {
        "modules": true
    },
    "env": {
        "es6": true,
        "browser": true
    },
    "rules": {
        "no-unused-vars": 2,
        "no-undef": 2,
        "eqeqeq": 2,
        "guard-for-in": 2,
        "no-extend-native": 2,
        "wrap-iife": 2,
        "new-cap": 2,
        "no-caller": 2,
        "no-multi-str": 0,
        "dot-notation": 0,
        "strict": [2, "global"],
        "valid-jsdoc": 1,
        "no-irregular-whitespace": 1,
        "no-multi-spaces": 2,
        "one-var": [2, "never"],
        "constructor-super": 2,
        "no-this-before-super": 2,
        "no-var": 2,
        "prefer-const": 1,
        "no-const-assign": 2
    },
    "globals": {
        "require": false,
        "module": false,
        "exports": false,
        "requireText": false
    }
}

[callumlocke]

What's the status of this now?

[AlbertoElias]

It's being released today or tomorrow, once different teams make sure we don't break their builds

[AlbertoElias]

This is now done