Issue 4864 - Start using ESLint for adblockpluschrome

chrome/ext/common.js

Index: chrome/ext/common.js
diff --git a/chrome/ext/common.js b/chrome/ext/common.js
index 011be377721d23d33f09b4b899dca7f0a0ec25ff..11ec937025432862900b3c4e1b90ffe6c1424115 100644
--- a/chrome/ext/common.js
+++ b/chrome/ext/common.js
@@ -17,14 +17,16 @@
 
 "use strict";
 
+(function()

[Sebastian Noack, 2017/03/29 17:14:11]

Apparently, it was decided to put IIFEs everywhere. From a comment above in this
review, block scoping is assumed to be a footgun with function definitions. How
is that? Named functions are block scoped in strict-mode, and since we ensure
strict-mode with ESLint, you cannot argue that somebody might forget to turn
strict-mode on, or do I miss something?

Anyway, you reverted other changes because they seem unrelated to you, how is
adding IIFEs here related to ESLint compliance, or is there an ESLint rule that
enforces them?

[kzar, 2017/03/30 07:20:41]

Apparently it's a syntax error to declare a named function inside of a block
scope like that. Chrome ignored it but Firefox didn't. This meant that some of
the adblockplusui/adblockpluscore (I forget) changes caused problems for Firefox
and we had to switch back to using IIFEs there.

As Wladimir pointed out sometimes we used a block like that, without declaring
named functions. While not a syntax error it was kind of easy to add a function
declaration without remembering to switch to a IIFE, so for that reason and
consistency we changed them for those too.
I would really rather not. Do you insist there?

[Sebastian Noack, 2017/03/30 18:57:56]

Chrome does not "ignore" the error but behaves correctly here. Function
declarations in nested blocks are supported in ES6, with the function name being
block-scoped and hoisted to the top of the lexical block, which is exactly what
Chrome does in strict mode. In ES5, however, the behavior of function
declarations in nested blocks was undefined. Firefox is apparently behind the
standard here, and I confirmed that in strict mode it is throwing an error. So
yeah, it seems we are back to IIFEs. Not only for consistence, but also because
this code has to run on Firefox soon. :(

[kzar, 2017/03/31 03:42:17]

Acknowledged.

 {
   // Workaround since HTMLCollection and NodeList didn't have iterator support
   // before Chrome 51.
   // https://bugs.chromium.org/p/chromium/issues/detail?id=401699
+  let arrayIterator = Array.prototype[Symbol.iterator];
   if (!(Symbol.iterator in HTMLCollection.prototype))
-    HTMLCollection.prototype[Symbol.iterator] = Array.prototype[Symbol.iterator];
+    HTMLCollection.prototype[Symbol.iterator] = arrayIterator;
   if (!(Symbol.iterator in NodeList.prototype))
-    NodeList.prototype[Symbol.iterator] = Array.prototype[Symbol.iterator];
+    NodeList.prototype[Symbol.iterator] = arrayIterator;
 
   /* Message passing */
 
@@ -46,4 +48,4 @@
 
   ext.getURL = chrome.extension.getURL;
   ext.i18n = chrome.i18n;
-}
+}());