Issue 3692 - Add base ESLint configuration

.eslintrc.json

+{
+  "extends": "eslint:recommended",
+  "env": {
+    "es6": true
+  },
+  "rules": {
+    "array-bracket-spacing": "error",
+    "arrow-spacing": "error",
+    "block-scoped-var": "error",
+    "block-spacing": "error",
+    "brace-style": ["error", "allman", {"allowSingleLine": true}],
+    "camelcase": ["error", {"properties": "never"}],
+    "comma-dangle": "error",
+    "comma-spacing": "error",
+    "comma-style": "error",
+    "computed-property-spacing": "error",

[Wladimir Palant, 2017/02/03 10:24:13]

It's a pity that the consensus was apparently to remove consistent-return, I
consider that rule very useful and an occasional explicit "return undefined" not
a bad thing.

[Sebastian Noack, 2017/02/03 12:49:32]

I generally agree that it makes the code flow more clear if a function that is
called normally, always returns explicitly, if there is a potential return
value. But Dave and I agree, that this doesn't hold true for hooks that are
designed for optional return values (webRequest API, Promise.then(), ours
messaging.Port, etc.). in that case, we feel that the redundant "return
undefined" makes our code more clumsy. While we tested the previously suggested
configuration (with consistent-return enabled) on our code base, we didn't find
any instance of the former scenario, but quite some instances where we agreed
that it would make our code less preferable.

[kzar, 2017/02/04 03:18:55]

I don't want to use the consistent-return rule for adblockpluschrome, like
Sebastian mentioned it's a pain there. (If you insist we can enable it generally
and then turn it off again in the adblockpluschrome configuration though.)

+    "curly": ["error", "multi-or-nest", "consistent"],
+    "eol-last": "error",
+    "func-call-spacing": "error",
+    "indent": ["error", 2, {"SwitchCase": 1, "ArrayExpression": "first"}],
+    "key-spacing": "error",
+    "keyword-spacing": "error",
+    "linebreak-style": "error",
+    "lines-around-directive": "error",
+    "max-len": ["error", 80, {"ignoreUrls": true}],
+    "new-parens": "error",
+    "no-array-constructor": "error",
+    "no-caller": "error",
+    "no-case-declarations": "off",

[Wladimir Palant, 2017/02/03 10:24:13]

Is this a good idea? Lexical declarations inside case clauses are disallowed for
a reason - a case clause doesn't have its own lexical scope, only the switch
statement does. So the following is an error because it redeclares variable x:

switch (foo)
{
  case 1:
    let x = 1;
    break;
  case 2:
    let x = 2;
    break;
}

The correct way of writing this requires an additional block:


switch (foo)
{
  case 1:
  {
    let x = 1;
    break;
  }
  case 2:
  {
    let x = 2;
    break;
  }
}

[Sebastian Noack, 2017/02/03 12:49:32]

There is no difference (or potential footgun) in this example, as the code
"breaks" before another "case" would be reached. Hence there is no way another
"case" is executed after the variable declaration, that would cause the variable
to be defined or not dependent on how that code is reached. Still ESLint would
complain, with no-case-declarations turned on. There is only a potential
footgun, if you have any code in a "case" that does not (or might not) break,
which is already prohibited by another rule. Also see my related comment on that
rule, in the earlier discussion, on the other review.

[Wladimir Palant, 2017/02/03 13:01:34]

Try it ;)

This code will actually error out at compile time because of "redeclaration of
let x." While technically there might be no issue, declaring the same let
variable multiple times within the same lexical scope isn't allowed.

[kzar, 2017/02/04 03:18:55]

I don't mind too much either way about this rule, so I've turned it back on for
now. Done.

[Sebastian Noack, 2017/02/04 16:43:37]

Wladimir is right that in his example omitting the braces will actually produce
a syntax error. But you don't need a linter to find syntax errors, and in
particular I'm not sure whether this justifies to add a redundant block to each
"case" in which a new variable is declared.

[Wladimir Palant, 2017/02/04 21:13:13]

The point of a linter is catching errors before you run the code. Besides, this
syntax error isn't the biggest issue - consider a variable declared in one case
statement and used in another. This is a valid construct, yet chances are that
somebody simply forgot to declare this variable in the second case statement.
This kind of interaction between different case statements is a highly
unexpected language "feature" which is why this rule and the "redundant" block
make lots of sense IMHO.

[Sebastian Noack, 2017/02/05 18:22:39]

I completely agree, having code that might fall through after a case statement,
is a footgun, not only when there is a variable declaration. However, as I told
you before, this is already catched by another rule, i.e. no-falltrough. Also I
just noticed that the redeclaration in your example will be detected by
no-redeclare. Both rules are turned on, as they are part of eslint:recommended.
So in fact, there aren't any issues, we need no-case-declaration for, to catch.

[Wladimir Palant, 2017/02/06 09:55:19]

That's not what I meant. Here is the actual code:

switch (foo)
{
  case 1:
    let x = 2;
    break;
  case 2:
    x += 4;
    break;
}

Feel free to test whether any other rules catch this, normally all you get is a
runtime error "ReferenceError: can't access lexical declaration `x' before
initialization" (only if that code path is actually executed).

[Sebastian Noack, 2017/02/06 13:54:43]

Regardless whether you put a redundant block around the first case or not (and
that is the only thing no-case-declarations would complain about here), you
still get the same ReferenceError at runtime. Your example hasn't much to do
with lexical declarations in case statements, but rather with usage of undefined
variables. If we decide to go ahead with no-undef, it would detect the issue
here, but that is another discussion. Either way no-case-declarations isn't any
useful here. So can we please finally agree to turn it off again?

[kzar, 2017/02/06 16:26:13]

Well honestly I don't mind too much either way. How about we turn
no-case-declarations off but leave no-undef on?

[Sebastian Noack, 2017/02/06 17:26:00]

Honestly, I cannot see how this is still a controversial decision. After
clearing up some wrong assumptions, we established that there is not a single
potential issue caught by no-case-declarations (that isn't redundantly
identified by other rules). At the same time the additional boilerplate enforced
by no-case-declarations is redundant and potentially inconsistent. Turning this
rule off, at the current point, should be a no-brainer. However, no-undef is a
whole different story and perhaps a more controversial rule.

+    "no-catch-shadow": "error",
+    "no-cond-assign": "off",

[Wladimir Palant, 2017/02/03 10:24:13]

Frankly, I'd rather not disable this. An assignment inside a condition is rarely
intended, and even when it is it's bad for readability.

[Sebastian Noack, 2017/02/03 12:49:32]

From the earlier discussion: While I might agree that assigning in an
if-statement sometimes results in somewhat cryptic code, I think for while-loops
following pattern is totally fine:

  let match;
  while (match = regex.exec(message))
  {
    doSomthing(match[1]);
  }

In fact, I think it is preferable over the alternative:

  while (true)
  {
    let match = regex.exec(message);
    if (!match)
      break;
    doSomething(match[1]);
  }

This is not only more verbose, but generally it is less obvious when/if the loop
terminates, if the exit condition isn't in the head.

Also note while testing this rule on our code base, we didn't find a single
inappropriate use of assignment in a condition statement, but just the pattern
above. So we agreed, that this rule would rather make our code worse.

[kzar, 2017/02/04 03:18:55]

I agree with Sebastian here, it's sometimes a useful pattern. I'd prefer to
leave this rule off as well.

+    "no-console": ["error", {"allow": ["warn", "error", "trace"]}],
+    "no-control-regex": "off",
+    "no-else-return": "error",
+    "no-eval": "error",
+    "no-extra-bind": "error",
+    "no-extra-label": "error",
+    "no-implied-eval": "error",
+    "no-labels": "error",
+    "no-lone-blocks": "error",
+    "no-lonely-if": "error",
+    "no-multi-spaces": "error",
+    "no-new-func": "error",
+    "no-new-object": "error",
+    "no-proto": "error",
+    "no-self-compare": "error",
+    "no-shadow": "error",
+    "no-trailing-spaces": "error",
+    "no-undef": "off",

[Wladimir Palant, 2017/02/03 10:24:13]

Why is this rule turned off? Flagging undeclared variables is the most important
feature of a linter. Any variables coming from external scopes should either be
covered by the environment or /* global */ comments.

[Sebastian Noack, 2017/02/03 12:49:31]

While we tested that rule on our code base, we noticed a huge amount of errors
indicating use of undefined variables. These include global names defined in
other scripts, names injected when bundling modules during the build process,
and built-in browser APIs. The required boilerplate to get rid of those errors
would be significant and repetitive, Dave had a go.

However, as long as the code path gets executed during testing, there is no way
that you use an undefined name unnoticed, as opposed to the more sneaky quality
issues catched by ESLint. So I don't think it's worth to add the necessary
boilerplate to make ESLint happy with no-undef turned on. This was the consensus
of the previous discussion.

[Wladimir Palant, 2017/02/03 13:01:34]

This indicates an issue. Built-in browser APIs and CommonJS should be covered by
a proper environment setting. If then we still have a huge amount of errors then
something is wrong with our code, different parts should normally be fairly
self-contained.
That boilerplate actually has some benefit: it shows human readers what external
dependencies there are.

[kzar, 2017/02/04 03:18:55]

I can see arguments either way for this rule and while I'd rather leave it off I
won't insist. So in the interest of progress I've turned it back on for now.
Done.

[Sebastian Noack, 2017/02/04 16:43:37]

IMO, the job of a linter is to ensure good coding practices, not necessarily to
find bugs. So I lend against any rule, that tends to generate false positives,
or requires additional boilerplate to avoid those, in particular if the issue
the rule aims to detect will come up during testing anyway.

But for the matter of progress, lets revisit this later. I would like to see the
complete changes to adblockpluschrome in order to make ESLint pass with this
configuration anyway, before approving it. So let's first see how bad the
related boilerplate turns out.

However, if there is any rule we cannot agree on (and that holds true as well
for the other controversial rule above, and any rule we are still going to
discuss), we should not use it. Let's try to enforce the rules we agree on,
rather than trying to make ESLint as strict as possible, in an infinite
bike-shedding discussion, resulting in unpopular compromises. After all this
will define the baseline for our coding practices that everybody has to accept.

[Wladimir Palant, 2017/02/04 21:13:13]

I see, that's the main reason for our disagreements it seems. Thing is,
JavaScript is a highly dynamic language which makes it hard to catch mistakes at
"compile time." IMHO, one large advantage of linting JavaScript is that you can
catch some coding mistakes before you even run the code. And the assumption that
each code path will be executed in testing is regularly being proven wrong.

[Sebastian Noack, 2017/02/05 18:22:39]

If we can catch some errors early, that is a nice side-effect of linting of
course. But I'd rather leverage the dynamic of JavaScript in order to keep the
code simple, concise and avoid repetition rather than limiting those benefits
through linting.

After all, accidental usage of undefined variables isn't the only cause of
potential errors. So there is no way around testing every code path anyway, in
order to make sure that it works, also if this might not always have happened.

Anyway, as I said before, I'm happy to defer this discussion, until we see the
exact boilerplate we'd have to add in order to use no-undef.

[kzar, 2017/02/06 06:21:01]

I've added the boilerplate required for no-undef in my branch now,
https://github.com/kzar/adblockpluschrome/tree/4864-eslint . (Not linking the
commit since I'm force-pushing to that quite often at the moment.)

The changes weren't as bad as I originally thought, I wouldn't mind leaving
no-undef on.

[Sebastian Noack, 2017/02/06 17:26:00]

Thanks. Indeed, the configuration for environment-specific globals is quite
straight-forward. It would be even better, though, if you'd add the "commonjs"
(includes "exports" and "requires") and "webextensions" (includes "chrome" and
"browser") environments, rather than explicitly listing the respective globals.

But the boilerplate to configure our own globals from other scripts is still too
much. This effects mostly places (like content scripts and the user interface)
where we don't organize the code in modules, which isn't great in the first
place. So how about bundling that code as modules as well? This will make the
code better organized, and eliminate the need for most globals, in one go.

[kzar, 2017/02/07 06:37:58]

Sure, we can improve on the work in adblockpluschrome (and I'm about to have a
look at your feedback on GitHub for that) but it's really a separate issue
(#4864). For now I'm just hoping we can find a common set of rules for this base
ESLint configuration so that we can get it pushed.

- It seems we agree that the burden of no-undef compliance isn't too large so
how about we leave it enabled?
- It seems no-case-declarations is contested so let's just leave it disabled.

Is that a suitable compromise that you'd both be happy for this to land? If so
I'd appreciate a looks good, if not please suggest a compromise that might work
for you both.

[Sebastian Noack, 2017/02/07 11:57:15]

The linter configuration defines our coding practices, and in order to evaluate
the suggested practices/configuration, I need to review the changes it implies
on our code, first. If you could put up a combined patch, with the changes for
adblokpluschrome, for review, that would be helpful. Doing codereview on GitHub
is quite inconvenient for me, also I'd prefer to review all changes together. As
for no-undef, I'm fine with it, under the condition that we significantly reduce
the usage of global variables in our code (e.g. through modules).

[kzar, 2017/02/07 17:07:51]

It's a WIP but here you go https://codereview.adblockplus.org/29374674/

[kzar, 2017/02/09 04:55:27]

Since you've now gone through and reviewed the adblockpluschrome changes would
you mind giving this base ESLint configuration a looks good? While I'm still in
the process of addressing your feedback I can't see anything there that should
block this review, and in fact those changes can't land before this does anyway.

+    "no-unneeded-ternary": "error",
+    "no-unused-vars": "off",
+    "no-useless-computed-key": "error",
+    "no-useless-concat": "error",
+    "no-useless-escape": "error",
+    "no-useless-return": "error",
+    "no-var": "error",
+    "no-warning-comments": "error",
+    "no-whitespace-before-property": "error",
+    "no-with": "error",
+    "object-curly-spacing": "error",
+    "object-shorthand": ["error", "always", {
+      "avoidExplicitReturnArrows": true
+    }],
+    "one-var": ["error", "never"],
+    "operator-assignment": "error",
+    "operator-linebreak": "error",
+    "padded-blocks": ["error", "never"],
+    "prefer-arrow-callback": "error",
+    "prefer-destructuring": ["error", {"array": false}],
+    "prefer-numeric-literals": "error",
+    "prefer-spread": "error",

[Wladimir Palant, 2017/02/03 10:24:13]

What about prefer-rest-params?

[Sebastian Noack, 2017/02/03 12:49:32]

Well spotted. We had it in there before, and I don't recall that we discussed
removing it, neither do I see why we should.

[kzar, 2017/02/04 03:18:55]

I omitted this rule since I assumed rest parameters wouldn't work for code like
the WebSocket wrapper in adblockpluschrome/include.preload.js. But I've just
checked and I think I was wrong since `(function(a, ...b) { }).length == 1`.
Done.

+    "quote-props": ["error", "as-needed"],
+    "quotes": ["error", "double", {"avoidEscape": true}],
+    "radix": "error",
+    "rest-spread-spacing": "error",
+    "semi": "error",
+    "semi-spacing": "error",
+    "space-before-function-paren": ["error", "never"],
+    "space-in-parens": "error",
+    "space-infix-ops": "error",
+    "space-unary-ops": "error",
+    "spaced-comment": "error",
+    "strict": ["error", "global"],
+    "valid-jsdoc": ["error", {
+      "requireParamDescription": false,
+      "requireReturn": false,
+      "requireReturnDescription": false
+    }],
+    "yield-star-spacing": "error",
+    "yoda": "error"
+  }
+}