Issue 4878 - Start using ESLint for adblockpluscore

.eslintrc.json

Index: .eslintrc.json
diff --git a/.eslintrc.json b/.eslintrc.json
new file mode 100644
index 0000000000000000000000000000000000000000..894fda25270f2eb020bd19f76ea6d6f7825598dd
--- /dev/null
+++ b/.eslintrc.json
@@ -0,0 +1,23 @@
+{
+  "extends": "eslint-config-eyeo",
+  "root": true,
+  "env": {
+    "shared-node-browser": true

[Wladimir Palant, 2017/02/21 11:54:02]

There is some context missing here, why not commonjs? I take it, commonjs
environment will always object on unused global variables. And this seems to be
perfectly fine when applied to actual CommonJS modules. I assume that this is
about lib/common.js which can also run in browser environment - a comment in
that file declaring its environment properly should do then.

Note that shared-node-browser seems to make little sense here, given that we
occasionally (Firefox and libadblockplus) run these files in an environment
which is neither the browser nor Node.js - all globals are being set explicitly
then.

[kzar, 2017/02/23 10:53:55]

Yep, using commonjs objects to unused global variables which results in the
following ESLint errors:

.../adblockpluscore/lib/elemHide.js
  24:8  error  'Utils' is assigned a value but never used  no-unused-vars

.../adblockpluscore/lib/elemHideEmulation.js
  33:5  error  'ElemHideEmulation' is assigned a value but never used 
no-unused-vars

.../adblockpluscore/lib/matcher.js
   25:16  error  'RegExpFilter' is assigned a value but never used   
no-unused-vars
  465:5   error  'defaultMatcher' is assigned a value but never used 
no-unused-vars

.../adblockpluscore/lib/synchronizer.js
  30:16  error  'CommentFilter' is assigned a value but never used 
no-unused-vars
Acknowledged.

[Wladimir Palant, 2017/03/02 14:06:41]

That's correct, this should be removed - I think Utils used to be necessary
before but they aren't any more.
Also correct, only assigning to exports.ElemHideEmulation is necessary here. But
it's probably a good idea to change the pattern into:

let ElemHideEmulation = ...;
exports.ElemHideEmulation = ElemHideEmulation;
Same as above.
Yes, this is a case where we should really drop the local variable.
Correct, no longer being used there.

[kzar, 2017/03/08 12:33:24]

Done.

+  },
+  "globals": {
+    "atob": true,
+    "exports": true,
+    "module": true,
+    "onShutdown": true,
+    "require": true,
+    "Buffer": true,

[Wladimir Palant, 2017/02/21 11:54:02]

This is wrong, Buffer isn't part of our usual environment - this is Node.js.
Unit tests are running in Node and declaring the correct environment via comment
in the file seems to be the best course of action (only one file affected IMHO).

[kzar, 2017/02/23 10:53:55]

Done.

+    "Cc": true,
+    "Ci": true,
+    "Cu": true,
+    "FileUtils": true,
+    "Services": true,
+    "URL": true,
+    "XPCOMUtils": true,

[Wladimir Palant, 2017/02/21 11:54:02]

Rather than declaring FileUtils, Services and XPCOMUtils here  even though they
aren't really globals, I'd suggest importing them properly:

  let {FileUtils} = Cu.import("resource://gre/modules/FileUtils.jsm", {});

This will need slight changes to lib/compat.js in adblockpluschrome. Long-term
goal is getting rid of these Firefox implementation details of course.

[kzar, 2017/02/23 10:53:55]

I'd rather do this afterwards as a separate change, the changes here (and to
adblockpluschrome + adblockplusui) are already huge enough as it is.

[Wladimir Palant, 2017/03/02 14:06:41]

File an issue?

[kzar, 2017/03/08 12:33:24]

Why not, done it after all. (See corresponding changes to the adblockpluschrome
review https://codereview.adblockplus.org/29374674/ )

+    "XMLHttpRequest": true
+  }
+}