Issue 3692 - Created global eslintrc

.eslintrc.json

Index: .eslintrc.json
===================================================================
new file mode 100644
--- /dev/null
+++ b/.eslintrc.json
@@ -0,0 +1,58 @@
+{
+  "extends": "eslint:recommended",
+  "rules": {
+    /* MDN General practices */

[kzar, 2016/09/16 10:33:51]

(It surprised me to see comments in a JSON file but it looks like eslint strips
them out
http://eslint.org/docs/user-guide/configuring#comments-in-configuration-files )

[juliandoucette, 2016/10/13 14:14:08]

Acknowledged.

Let me know if you think we should remove them.

+    "no-else-return": "warn",
+    "no-console": "warn",
+    "no-debugger": "warn",
+    "no-alert": "warn",

[kzar, 2016/09/16 10:33:50]

There are some valid uses of alert in adblockpluschrome at least, but I can't
think of any valid uses of the other three above. Perhaps they should instead be
considered errors?

[Wladimir Palant, 2016/09/16 11:37:06]

Actually, using console.error() can be valid when exceptions need to be
reported. In fact, the console object is used several times in adblockpluschrome
repository - all perfectly valid use cases.

So, how are we going to deal with this?

1) Just leave it as is, it's "only" warnings - not a good idea IMHO, as it
trains people to ignore warnings. Ideally, no warnings or errors should be
produced unless there is a real issue.

2) Override the warnings in all the relevant code places - in adblockpluschrome
that's two files using alert() and six files using console. IMHO, too many
exceptions make a rule questionable.

3) Drop these rules and leave it to reviewers to spot debugging code (which
could be using Cu.reportError() or dump() as well).

[kzar, 2016/09/16 12:12:57]

Hmm good point. I guess I'm easy between options 2 and 3, but I agree about 1
being a bad idea.

[juliandoucette, 2016/10/13 14:14:10]

Acknowledged.

I changed this to rely on eslint:recommends for no-debugger and no-console and
error on "no-alert". 

Let me know if you think there are too many exceptions to this rule.

+    "linebreak-style": ["warn", "unix"],

[kzar, 2016/09/16 10:33:50]

Shouldn't linebreak-style be an error too, or am I missing a valid use case?

[juliandoucette, 2016/10/13 14:14:09]

Done.

+    "indent": ["warn", 2],
+    "valid-jsdoc": "warn",
+    "eol-last": "warn",

[Wladimir Palant, 2016/09/16 08:07:42]

This is already part of eslint:recommended, we don't need to duplicate it. If
the point was turning an error into a warning - I don't see why we should,
people tend to ignore warnings.

[juliandoucette, 2016/10/13 14:14:08]

Done.

+    "vars-on-top": "off",

[Wladimir Palant, 2016/09/16 08:07:43]

This isn't part of eslint:recommended, no point switching it off.

[juliandoucette, 2016/10/13 14:14:08]

Done.

+    /* MDN Naming and formatting code */
+    "no-irregular-whitespace": "warn",

[Wladimir Palant, 2016/09/16 08:07:43]

Like above, this is already part of eslint:recommended.

[kzar, 2016/09/16 10:33:51]

Yea and it should really be an error. One time a zero-width space character did
break some of our code! It was Python code so not totally relevant, but still
scary. It was invisible in Rietveld so passed straight through review!

I set up emacs to highlight those red after that, if you're interested in doing
that too here's how:
https://github.com/kzar/emacs.d/blob/master/init.el#L181-L191

[juliandoucette, 2016/10/13 14:14:10]

Done.

+    "no-trailing-spaces": "warn",
+    "max-len": ["warn", 80],
+    "brace-style": ["warn", "allman"],
+    /* MDN JavaScript practices */
+    "no-eq-null": "off",

[Wladimir Palant, 2016/09/16 08:07:42]

This isn't part of eslint:recommended, no point switching it off.

[juliandoucette, 2016/10/13 14:14:09]

Done.

+    "no-redeclare": "error",

[Wladimir Palant, 2016/09/16 08:07:43]

This is already part of eslint:recommended.

[kzar, 2016/09/16 10:33:51]

To play devil's advocate perhaps it's good to specify everything explicitly?
Then should eslint:recommended change in the future we won't be surprised. (If
two developers had different versions it's possible the two defaults could clash
too.)

[Wladimir Palant, 2016/09/16 11:37:06]

I think that eslint:recommended usually changes when eslint's capabilities
change, so we'll have to adjust one way or another. eslint:recommended actually
makes it easier to catch up here.

It definitely makes things easier that our rules are based on a "common
standard" and we merely document deviations from it. If somebody wants to learn
everything important about our coding style - it's only going to be a few dozen
rules and not a few hundred.

[kzar, 2016/09/16 12:12:57]

Fair enough.

[juliandoucette, 2016/10/13 14:14:08]

Done.

+    "consistent-return": "error",

[kzar, 2016/09/16 10:33:51]

Does everyone agree with consistent-return? To me it seems kind of verbose to
add a `return undefined;` to the end of functions when that happens implicitly.

[Wladimir Palant, 2016/09/16 11:37:06]

Yes, consistent-return is important and actually something we already do
consistently to my knowledge. I cannot imagine a situation where you would have
`return undefined` at the end of a function, maybe `return null` (which already
isn't equivalent to not returning anything). And it is a very common mistake to
forget returning a value on one of the branches of the function.

Note that this doesn't require you having an explicit return at the end of the
function if none of its branches return a value.

[juliandoucette, 2016/10/13 14:14:08]

Done.

[kzar, 2017/01/31 10:17:48]

One example where I think consistent-return might be a bad idea is the
onHeadersReceived listener in adblockpluschrome/lib/csp.js[1]. There we check if
any blocking filters match, if not we don't do anything (implicitly returning
`undefined`), otherwise we return the headers array with our CSP header pushed
on the end.

Initially I thought the rule was OK, I simply changed the logic around so first
we `return;` if there is no matching blocking filter. Problem was _even that_
wasn't enough for the consistent-return rule, I had to do explicitly `return
undefined;` instead.

IMO that's pretty ugly, I'd rather not litter our code with `return
undefined;`'s and so I'd rather not use the consistent-return rule. What do you
think?

[1] -
https://github.com/adblockplus/adblockpluschrome/blob/master/lib/csp.js#L24-L50

+    "no-undef": "error",

[Wladimir Palant, 2016/09/16 08:07:43]

This is already part of eslint:recommended.

[juliandoucette, 2016/10/13 14:14:09]

Done.

+    "no-unused-vars": "warn",

[Wladimir Palant, 2016/09/16 08:07:43]

The reason I switched this rule off: it produces tons of false positives. For
example, with your settings `new Promise((resolve, reject) => resolve(true))`
will warn that `reject` is unused. I don't consider leaving out parameters that
are being passed in but not used a good measure.

[kzar, 2016/09/16 10:33:51]

Agreed, it's better to not use a parameter than silently drop it.

[juliandoucette, 2016/10/13 14:14:09]

Done.

+    "no-array-constructor": "warn",
+    "no-new-object": "warn",
+    /* ABP General */
+    "no-warning-comments": "warn",
+    /* Palant recommends */

[kzar, 2016/09/16 10:33:50]

Nit: Mind adding a newline before each new section?

[juliandoucette, 2016/10/13 14:14:09]

Done.

+    "array-bracket-spacing": "warn",
+    "arrow-spacing": "warn",
+    "comma-spacing": "warn",
+    "comma-style": "warn",
+    "computed-property-spacing": "warn",
+    "key-spacing": "warn",
+    "keyword-spacing": "warn",
+    "new-parens": "warn",
+    "no-spaced-func": "warn",
+    "no-var": "warn",

[kzar, 2016/09/16 10:33:51]

We need to use var in adblockpluschrome in some places, for scripts that aren't
transpiled like content scripts.

[Wladimir Palant, 2016/09/16 11:37:06]

This is an ES6 config, and we are already discussing what we should do with ES5
environments. Overriding this rule in a file is definitely possible, but
personally I'd favor just transpiling everything.

+    "no-whitespace-before-property": "warn",
+    "object-curly-spacing": "warn",
+    "operator-assignment": "warn",
+    "operator-linebreak": "warn",
+    "padded-blocks": ["warn", "never"],
+    "prefer-rest-params": "warn",
+    "prefer-spread": "warn",

[kzar, 2016/09/16 10:33:51]

I don't mind using the spread operator instead of .apply when possible but that
would require some changes to our tooling for adblockpluschrome at least.
(Perhaps we could improve jshydra to support those?)

[Wladimir Palant, 2016/09/16 11:37:06]

We definitely could and should.

Note that we are quickly getting to the point where transpiling in extensions is
only necessary to support Safari. Chrome 49 and Edge support all ES6 features we
need.

[kzar, 2016/09/16 12:12:57]

Perhaps I'll give it a go if I ever find time. I very briefly looked at it
before but wasn't sure how to do it without binding the function. That's
offtopic here anyway so I've opened an issue
https://issues.adblockplus.org/ticket/4443

[juliandoucette, 2016/10/13 14:14:09]

Acknowledged.

+    "require-yield": "warn",
+    "quotes": ["warn", "double", {"avoidEscape": true}],
+    "semi": "warn",
+    "semi-spacing": "warn",
+    "space-before-function-paren": ["warn", "never"],
+    "space-in-parens": "warn",
+    "space-infix-ops": "warn",
+    "space-unary-ops": "warn",
+    "spaced-comment": "warn",
+    "yield-star-spacing": "warn"
+  }
+}