Issue 5685 - Pass ESLint

Issue 5685 - Pass ESLint

[hub, 2017-09-15 19:44:01]

https://codereview.adblockplus.org/29545700/diff/29545701/.eslintignore
File .eslintignore (right):

https://codereview.adblockplus.org/29545700/diff/29545701/.eslintignore#newcode3
.eslintignore:3: lib/basedomain.js
This is a third-party file. Leaving it alone for now.

https://codereview.adblockplus.org/29545700/diff/29545701/.eslintignore#newcode4
.eslintignore:4: lib/compat.js
This one mostly pass but has some issues: it export some functions, but since
there is a global env "commonjs" exported can't be used. And one can't disable
env.

https://codereview.adblockplus.org/29545700/diff/29545701/lib/compat.js
File lib/compat.js (right):

https://codereview.adblockplus.org/29545700/diff/29545701/lib/compat.js#newco...
lib/compat.js:58: nsIHttpChannel,
This one was a bit of a corner case. eslint style want us to use the "method"
short hand. But this cause to make nsIHttpChannel not be `instanceof
nsIHttpChannel`, causing failures.

So we declare it as an ES6 class.

[sergei, 2017-09-18 07:59:05]

https://codereview.adblockplus.org/29545700/diff/29545701/.eslintignore
File .eslintignore (right):

https://codereview.adblockplus.org/29545700/diff/29545701/.eslintignore#newcode3
.eslintignore:3: lib/basedomain.js
On 2017/09/15 19:44:00, hub wrote:
> This is a third-party file. Leaving it alone for now.

Acknowledged.

https://codereview.adblockplus.org/29545700/diff/29545701/.eslintignore#newcode4
.eslintignore:4: lib/compat.js
On 2017/09/15 19:44:00, hub wrote:
> This one mostly pass but has some issues: it export some functions, but since
> there is a global env "commonjs" exported can't be used. And one can't disable
> env.

If it's because of definition of `require` then would it be possible to move it
into a separate file and ignore only this separate file?
Maybe it's for another commit, though.

https://codereview.adblockplus.org/29545700/diff/29545701/lib/.eslintrc.json
File lib/.eslintrc.json (right):

https://codereview.adblockplus.org/29545700/diff/29545701/lib/.eslintrc.json#...
lib/.eslintrc.json:18: "Ci": true
Is it necessary to have Cc and Ci for libadblockplus/lib? It seems they are not
used there as.

https://codereview.adblockplus.org/29545700/diff/29545701/lib/compat.js
File lib/compat.js (right):

https://codereview.adblockplus.org/29545700/diff/29545701/lib/compat.js#newco...
lib/compat.js:32: function importAll(module, globalObj)
It seems this function is not used, could you just remove it?

https://codereview.adblockplus.org/29545700/diff/29545701/lib/compat.js#newco...
lib/compat.js:58: nsIHttpChannel,
On 2017/09/15 19:44:00, hub wrote:
> This one was a bit of a corner case. eslint style want us to use the "method"
> short hand. But this cause to make nsIHttpChannel not be `instanceof
> nsIHttpChannel`, causing failures.
> 
> So we declare it as an ES6 class.

Acknowledged.

https://codereview.adblockplus.org/29545700/diff/29545701/lib/compat.js#newco...
lib/compat.js:342: }.bind(this);
Could you also turn this function into the arrow function, so this `bind` is not
required.

https://codereview.adblockplus.org/29545700/diff/29545701/lib/compat.js#newco...
lib/compat.js:415: if (!Array.prototype.find)
Is it possible to remove this polyfill? We are already using a newer V8, I would
just remove it.

https://codereview.adblockplus.org/29545700/diff/29545701/lib/init.js
File lib/init.js (right):

https://codereview.adblockplus.org/29545700/diff/29545701/lib/init.js#newcode52
lib/init.js:52: let {Subscription, DownloadableSubscription} =
should it and a couple of lines below be rather const?

[hub, 2017-09-18 12:43:05]

https://codereview.adblockplus.org/29545700/diff/29545701/.eslintignore
File .eslintignore (right):

https://codereview.adblockplus.org/29545700/diff/29545701/.eslintignore#newcode4
.eslintignore:4: lib/compat.js
On 2017/09/18 07:59:04, sergei wrote:
> On 2017/09/15 19:44:00, hub wrote:
> > This one mostly pass but has some issues: it export some functions, but
since
> > there is a global env "commonjs" exported can't be used. And one can't
disable
> > env.
> 
> If it's because of definition of `require` then would it be possible to move
it
> into a separate file and ignore only this separate file?
> Maybe it's for another commit, though.

eventually we'll move to webpack and this will be made obsolete. in the mean
time we can do that.

https://codereview.adblockplus.org/29545700/diff/29545701/lib/.eslintrc.json
File lib/.eslintrc.json (right):

https://codereview.adblockplus.org/29545700/diff/29545701/lib/.eslintrc.json#...
lib/.eslintrc.json:18: "Ci": true
On 2017/09/18 07:59:04, sergei wrote:
> Is it necessary to have Cc and Ci for libadblockplus/lib? It seems they are
not
> used there as.

not in here, indeed.

https://codereview.adblockplus.org/29545700/diff/29545701/lib/compat.js
File lib/compat.js (right):

https://codereview.adblockplus.org/29545700/diff/29545701/lib/compat.js#newco...
lib/compat.js:32: function importAll(module, globalObj)
On 2017/09/18 07:59:05, sergei wrote:
> It seems this function is not used, could you just remove it?

Done.

https://codereview.adblockplus.org/29545700/diff/29545701/lib/compat.js#newco...
lib/compat.js:342: }.bind(this);
On 2017/09/18 07:59:05, sergei wrote:
> Could you also turn this function into the arrow function, so this `bind` is
not
> required.

Done.

https://codereview.adblockplus.org/29545700/diff/29545701/lib/compat.js#newco...
lib/compat.js:415: if (!Array.prototype.find)
On 2017/09/18 07:59:05, sergei wrote:
> Is it possible to remove this polyfill? We are already using a newer V8, I
would
> just remove it.

Done.

https://codereview.adblockplus.org/29545700/diff/29545701/lib/init.js
File lib/init.js (right):

https://codereview.adblockplus.org/29545700/diff/29545701/lib/init.js#newcode52
lib/init.js:52: let {Subscription, DownloadableSubscription} =
On 2017/09/18 07:59:05, sergei wrote:
> should it and a couple of lines below be rather const?

Done.

[sergei, 2017-09-18 14:56:48]

LGTM