Issue 6717 - Part 1: run eslint through npm

Issue 6717 - Part 1: run eslint through npm

[hub, 2018-06-15 20:21:01]

This is part 1 towards running the test doing `npm test`. After fixing a few
linting errors, I felt the bigger urge to address this problem.

[Sebastian Noack, 2018-06-15 21:34:26]

https://codereview.adblockplus.org/29808587/diff/29808591/package.json
File package.json (right):

https://codereview.adblockplus.org/29808587/diff/29808591/package.json#newcode11
package.json:11: "eslint": "eslint *.js lib/ qunit/ ext/ chrome/",
For reference, in adblockplusui they called the script just "lint". Perhaps, we
might want to keep that consistent.

[hub, 2018-06-17 23:06:09]

https://codereview.adblockplus.org/29808587/diff/29808591/package.json
File package.json (right):

https://codereview.adblockplus.org/29808587/diff/29808591/package.json#newcode11
package.json:11: "eslint": "eslint *.js lib/ qunit/ ext/ chrome/",
On 2018/06/15 21:34:26, Sebastian Noack wrote:
> For reference, in adblockplusui they called the script just "lint". Perhaps,
we
> might want to keep that consistent.

But in adblockpluscore it is called eslint (which is what I based it off). So
either or needs to be fixed to be consistent. I don't care either way, but
consistency.

[hub, 2018-06-18 12:15:57]

https://codereview.adblockplus.org/29808587/diff/29808591/package.json
File package.json (right):

https://codereview.adblockplus.org/29808587/diff/29808591/package.json#newcode11
package.json:11: "eslint": "eslint *.js lib/ qunit/ ext/ chrome/",
On 2018/06/17 23:06:08, hub wrote:
> On 2018/06/15 21:34:26, Sebastian Noack wrote:
> > For reference, in adblockplusui they called the script just "lint". Perhaps,
> we
> > might want to keep that consistent.
> 
> But in adblockpluscore it is called eslint (which is what I based it off). So
> either or needs to be fixed to be consistent. I don't care either way, but
> consistency.

Even libadblockplus use `eslint` as the script name.

[hub, 2018-07-13 14:15:29]

the task is now "lint" and not "eslint". We require eslint 4. (the migration to
eslint 5 will be done next, but it fails now with eslint 5)

[Sebastian Noack, 2018-07-13 15:04:08]

https://codereview.adblockplus.org/29808587/diff/29829574/package.json
File package.json (right):

https://codereview.adblockplus.org/29808587/diff/29829574/package.json#newcode7
package.json:7: "eslint": "^4.19.0",
We want to update to ESLint 5, don't we? Moreover, is it even necessary to
redundantly specify the eslint version here, since it's already given as
peerDependency in eslint-config-eyeo?

[hub, 2018-07-13 15:05:47]

https://codereview.adblockplus.org/29808587/diff/29829574/package.json
File package.json (right):

https://codereview.adblockplus.org/29808587/diff/29829574/package.json#newcode7
package.json:7: "eslint": "^4.19.0",
On 2018/07/13 15:04:08, Sebastian Noack wrote:
> We want to update to ESLint 5, don't we? Moreover, is it even necessary to
> redundantly specify the eslint version here, since it's already given as
> peerDependency in eslint-config-eyeo?

I have moved on to ^5.1.0.
When the peerDependency in eslint-config-eyeo, we can ditch it here.

[Sebastian Noack, 2018-07-13 15:08:26]

https://codereview.adblockplus.org/29808587/diff/29829574/package.json
File package.json (right):

https://codereview.adblockplus.org/29808587/diff/29829574/package.json#newcode7
package.json:7: "eslint": "^4.19.0",
On 2018/07/13 15:05:47, hub wrote:
> On 2018/07/13 15:04:08, Sebastian Noack wrote:
> > We want to update to ESLint 5, don't we? Moreover, is it even necessary to
> > redundantly specify the eslint version here, since it's already given as
> > peerDependency in eslint-config-eyeo?
> 
> I have moved on to ^5.1.0.
> When the peerDependency in eslint-config-eyeo, we can ditch it here.

Let's update eslint-config-eyeo [1] first, so that we don't need to change the
configuration here twice.

[1]: https://hg.adblockplus.org/codingtools/file/master/eslint-config-eyeo

[hub, 2018-07-13 16:12:43]

https://codereview.adblockplus.org/29808587/diff/29829574/package.json
File package.json (right):

https://codereview.adblockplus.org/29808587/diff/29829574/package.json#newcode7
package.json:7: "eslint": "^4.19.0",
On 2018/07/13 15:08:26, Sebastian Noack wrote:
> On 2018/07/13 15:05:47, hub wrote:
> > On 2018/07/13 15:04:08, Sebastian Noack wrote:
> > > We want to update to ESLint 5, don't we? Moreover, is it even necessary to
> > > redundantly specify the eslint version here, since it's already given as
> > > peerDependency in eslint-config-eyeo?
> > 
> > I have moved on to ^5.1.0.
> > When the peerDependency in eslint-config-eyeo, we can ditch it here.
> 
> Let's update eslint-config-eyeo [1] first, so that we don't need to change the
> configuration here twice.
> 
> [1]: https://hg.adblockplus.org/codingtools/file/master/eslint-config-eyeo

Acknowledged.

[hub, 2018-08-07 21:14:45]

We are actually blocked by this review
https://codereview.adblockplus.org/29829605/ and the matching upload to npm.

[kzar, 2018-08-08 11:15:20]

https://codereview.adblockplus.org/29808587/diff/29829596/README.md
File README.md (right):

https://codereview.adblockplus.org/29808587/diff/29829596/README.md#newcode93
README.md:93: You can lint the code using [ESLint](http://eslint.org).
Would you mind adding a link to our configuration here?

https://codereview.adblockplus.org/29808587/diff/29829596/ext/background.js
File ext/background.js (right):

https://codereview.adblockplus.org/29808587/diff/29829596/ext/background.js#n...
ext/background.js:99: return undefined;
This looks like an unrelated change? If not could you explain it?

[hub, 2018-08-14 11:37:04]

https://codereview.adblockplus.org/29808587/diff/29829596/README.md
File README.md (right):

https://codereview.adblockplus.org/29808587/diff/29829596/README.md#newcode93
README.md:93: You can lint the code using [ESLint](http://eslint.org).
On 2018/08/08 11:15:19, kzar wrote:
> Would you mind adding a link to our configuration here?

reworded a bit the whole paragraph and added the link back.

https://codereview.adblockplus.org/29808587/diff/29829596/ext/background.js
File ext/background.js (right):

https://codereview.adblockplus.org/29808587/diff/29829596/ext/background.js#n...
ext/background.js:99: return undefined;
On 2018/08/08 11:15:19, kzar wrote:
> This looks like an unrelated change? If not could you explain it?

Without it eslint show an error

  81:12  error  Expected getter 'url' to always return a value  getter-return

[Sebastian Noack, 2018-08-14 19:16:56]

https://codereview.adblockplus.org/29808587/diff/29829574/package.json
File package.json (right):

https://codereview.adblockplus.org/29808587/diff/29829574/package.json#newcode7
package.json:7: "eslint": "^4.19.0",
On 2018/07/13 15:04:08, Sebastian Noack wrote:
> [...] is it even necessary to redundantly specify
> the eslint version here, since it's already given as
> peerDependency in eslint-config-eyeo?

What is about this comment?

[hub, 2018-08-14 19:35:16]

https://codereview.adblockplus.org/29808587/diff/29829574/package.json
File package.json (right):

https://codereview.adblockplus.org/29808587/diff/29829574/package.json#newcode7
package.json:7: "eslint": "^4.19.0",
On 2018/08/14 19:16:55, Sebastian Noack wrote:
> On 2018/07/13 15:04:08, Sebastian Noack wrote:
> > [...] is it even necessary to redundantly specify
> > the eslint version here, since it's already given as
> > peerDependency in eslint-config-eyeo?
> 
> What is about this comment?

it is needed. Without it, I get:

````
$ npm install
added 1 package in 0.648s
[ no eslint installed ]
$ npm install
npm WARN eslint-config-eyeo@2.1.0 requires a peer of eslint@^5.1 but none was
installed.

up to date in 0.083s
````

If I add it, all is good.

[Sebastian Noack, 2018-08-14 19:54:09]

LGTM