Allow URLSearchParams global in eslint config

[erikvold]

I'd like to be able to use URLSearchParams in some tests for #6833 but our eslint config complains about it.

[sebastian]

The tests (except those in test/browser/) run on nodejs, where there is no global URLSearchParams object, or do I miss something?

[erikvold]

It looks like we just need to add browser as a supported environment here.

So change:

env: {
    es6: true
  },

to

env: {
    es6: true,
    browser: true
  },

[erikvold]

Perhaps it makes more sense to add this for each project which is actually run in the browser. So extend the adblockpluscore repo eslintrc.json file with the change in the previous comment instead of changing our root config. Which sounds best?

[erikvold]

Replying to sebastian:

The tests (except those in test/browser/) run on nodejs, where there is no global URLSearchParams object, or do I miss something?

Node version 10.1.0 has the URLSearchParams global.

[sebastian]

We shouldn't add env.browser = true for code that runs with Node.js. This would relax the rules too much, potentially causing future mistakes to be missed. If newer versions of Node.js have the URLSearchParams global than ESLint must allow usage of that global with just env.node = true. If the latest version of ESLint doesn't yet, we should file a bug, and for the time being we could add URLSearchParams to the whitelisted globals (instead of enabling env.browser).

However, note that as documented in the README, adblockpluscore is still compatible with Node.js >= 7, and unless there is a very strong case, I'd rather not require anything higher than Node.js 8, since that is the version that comes with current Linux distributions (e.g. Ubuntu 17.10 and Debian Buster).

[erikvold]

Replying to sebastian:

We shouldn't add env.browser = true for code that runs with Node.js. This would relax the rules too much, potentially causing future mistakes to be missed. If newer versions of Node.js have the URLSearchParams global than ESLint must allow usage of that global with just env.node = true. If the latest version of ESLint doesn't yet, we should file a bug, and for the time being we could add URLSearchParams to the whitelisted globals (instead of enabling env.browser).

Looks like env.node = true should work ​https://github.com/sindresorhus/globals/blob/b044d50/globals.json#L1020

However, note that as documented in the README, adblockpluscore is still compatible with Node.js >= 7, and unless there is a very strong case, I'd rather not require anything higher than Node.js 8, since that is the version that comes with current Linux distributions (e.g. Ubuntu 17.10 and Debian Buster).

Looks like it was added in Node 7.5.0 ​https://nodejs.org/docs/latest-v7.x/api/url.html#url_class_urlsearchparams

[sebastian]

I tested on Node.js 8.5.0 and there is no URLSearchParams global but require("url").URLSearchParams works. Could you use that for compatibility? Also please update the README to require Node.js >= 7.5. But we wouldn't need to touch the linter configuration then.

[erikvold]

Replying to sebastian:

I tested on Node.js 8.5.0 and there is no URLSearchParams global but require("url").URLSearchParams works. Could you use that for compatibility? Also please update the README to require Node.js >= 7.5. But we wouldn't need to touch the linter configuration then.

Ah yes, I'll just use require("url").URLSearchParams for #6833 then.