#7044 closed change (rejected)

Allow URLSearchParams global in eslint config

Reported by: erikvold Assignee:
Priority: Unknown Milestone:
Module: Unknown Keywords:
Cc: sebastian Blocked By:
Blocking: Platform: Unknown / Cross platform
Ready: no Confidential: no
Tester: Unknown Verified working: no
Review URL(s):

Description

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

Change History (11)

comment:1 Changed 13 months ago by erikvold

  • Blocking 6833 added

comment:2 follow-up: Changed 13 months ago by sebastian

  • Cc sebastian added

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

comment:3 Changed 13 months ago by sebastian

  • Blocking 6833 removed

comment:4 Changed 13 months ago by 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
  },

comment:5 Changed 13 months ago by 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?

comment:6 in reply to: ↑ 2 Changed 13 months ago by 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.

comment:7 follow-up: Changed 13 months ago by 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).

comment:8 in reply to: ↑ 7 ; follow-up: Changed 13 months ago by 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

comment:9 in reply to: ↑ 8 ; follow-up: Changed 13 months ago by 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.

comment:10 in reply to: ↑ 9 Changed 13 months ago by 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.

Last edited 13 months ago by erikvold (previous) (diff)

comment:11 Changed 13 months ago by sebastian

  • Resolution set to rejected
  • Status changed from new to closed
Note: See TracTickets for help on using tickets.