New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

url: expose the WHATWG URL API globally #18281

Closed
wants to merge 1 commit into
base: master
from

Conversation

@targos
Member

targos commented Jan 21, 2018

Install URL and URLSearchParams on the global object, like they can be
found in browser environments.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

url

/cc @nodejs/tsc @nodejs/url @jasnell

Quick CI: https://ci.nodejs.org/job/node-test-pull-request-lite/106/
Quick CI: https://ci.nodejs.org/job/node-test-pull-request-lite/107/

@vkurchatkin

This comment has been minimized.

Member

vkurchatkin commented Jan 21, 2018

I'm -0 on this, but I think that it's a leat preferable to schedule this for a major release.

@targos targos added the semver-major label Jan 21, 2018

@targos

This comment has been minimized.

Member

targos commented Jan 21, 2018

I think that it's a leat preferable to schedule this for a major release.

I agree. I forgot to put the semver-major label.

@cjihrig

This comment has been minimized.

Contributor

cjihrig commented Jan 21, 2018

I'd rather not add more globals, but I won't block this if people think it's the right thing to do.

@apapirovski

apapirovski approved these changes Jan 21, 2018 edited

Makes complete sense to me. Very much in favour. 👍

(Even though with this being semver-major, my vote doesn't make much of a difference.)

@addaleax

I think we might want a lint rule like the one we have for Buffer which verifies that internal modules don’t rely on the global and instead do explicitly require url?

@targos

This comment has been minimized.

Member

targos commented Jan 21, 2018

@addaleax we don't need a special rule. ESLint already considers uses of global URL an error.

@mcollina

This comment has been minimized.

Member

mcollina commented Jan 21, 2018

I'm +0 on this. The implementation is ok, but I would not add more globals, but I'm not objecting either as it's very common on web platform.

If we are doing this, I would prefer if we started using it everywhere, even internally (https://github.com/nodejs/node/blob/master/lib/_http_client.js#L26), and gently phase away the old url parser. If we think this is the way to go, there is very little incentive for the community to adopt it if we do not use it internally.

@TimothyGu

🎉

@bnoordhuis

I personally don't have a problem with exposing them as globals. Next up: TextEncoder?

I'll get my coat.

@@ -447,20 +431,23 @@ console.log(JSON.stringify(myURLs));
### Class: URLSearchParams
<!-- YAML
added: v7.5.0
changes:
- version: REPLACEME
pr-url: REPLACEME

This comment has been minimized.

@evanlucas

evanlucas Jan 22, 2018

Member

we should be able to also set this, shouldn't we?

<!-- YAML
added: v7.0.0
changes:
- version: REPLACEME
pr-url: REPLACEME

This comment has been minimized.

@evanlucas

evanlucas Jan 22, 2018

Member

we should be able to go ahead and set this, no?

This comment has been minimized.

@targos

targos Jan 22, 2018

Member

thanks. updated

@targos

This comment has been minimized.

@jasnell

Very happy to see this come back around. It wasn't quite as popular an idea the first time I suggested it :-)

@ChALkeR

This comment has been minimized.

Member

ChALkeR commented Jan 22, 2018

-0 for reasons described in nodejs/node-eps#28 (comment) (my and others comments). I don't think that things changed much since that discussion.

I won't block this though if this is what others want.

@@ -327,6 +328,24 @@
setupInspector(originalConsole, wrappedConsole, Module);
}
function setupGlobalURL() {
const { URL, URLSearchParams } = NativeModule.require('internal/url');

This comment has been minimized.

@silverwind

silverwind Jan 22, 2018

Contributor

Does this impact startup performance, and if so, could it be required on use possibly?

This comment has been minimized.

@TimothyGu

TimothyGu Jan 22, 2018

Member

No, as this module is already being imported on startup:

// Ensure setURLConstructor() is called before the native
// URL::ToObject() method is used.
NativeModule.require('internal/url');

url: expose the WHATWG URL API globally
Install URL and URLSearchParams on the global object, like they can be
found in browser environments.

PR-URL: #18281
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@targos

This comment has been minimized.

@targos

This comment has been minimized.

Member

targos commented Jan 30, 2018

I'm not sure what's going on with AIX but it does not seem related.
Given all the +1s, I'm going to land this tomorrow if there are no explicit objections.

@Trott

This comment has been minimized.

Member

Trott commented Jan 30, 2018

Trying again on AIX. Given all the problems that have erupted on CI in the last month or two, I'm kinda -1 on landing something if we can't get CI to build/run on a platform. But I'm optimistic this is fixed now, so hopefully no problems...

CI for AIX: https://ci.nodejs.org/job/node-test-commit-aix/12189/

@mhdawson

This comment has been minimized.

Member

mhdawson commented Jan 31, 2018

@Trott thanks for restarting AIX. I fixed up one of the machines yesterday. There were a bunch of stale processes. I see the run you launched is green :)

@targos

This comment has been minimized.

Member

targos commented Jan 31, 2018

Landed in 3124146

@targos targos closed this Jan 31, 2018

@targos targos deleted the targos:global-url branch Jan 31, 2018

targos added a commit that referenced this pull request Jan 31, 2018

url: expose the WHATWG URL API globally
Install URL and URLSearchParams on the global object, like they can be
found in browser environments.

PR-URL: #18281
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

kcaulfield94 added a commit to kcaulfield94/node that referenced this pull request Feb 2, 2018

url: expose the WHATWG URL API globally
Install URL and URLSearchParams on the global object, like they can be
found in browser environments.

PR-URL: nodejs#18281
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@bfred-it

This comment has been minimized.

bfred-it commented Feb 2, 2018

I'm a nobody here, but I hope to see APIs shared across platform in the global scope so I don't have to mess with the browser field and "redirect" files every time I want to publish a cross-platform package.

msoechting added a commit to hpicgs/node that referenced this pull request Feb 5, 2018

url: expose the WHATWG URL API globally
Install URL and URLSearchParams on the global object, like they can be
found in browser environments.

PR-URL: nodejs#18281
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

msoechting added a commit to hpicgs/node that referenced this pull request Feb 7, 2018

url: expose the WHATWG URL API globally
Install URL and URLSearchParams on the global object, like they can be
found in browser environments.

PR-URL: nodejs#18281
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

@jdalton jdalton referenced this pull request Mar 15, 2018

Closed

module: add createRequireFunction method #19360

4 of 4 tasks complete
@guybedford

This comment has been minimized.

Contributor

guybedford commented Apr 24, 2018

I'm just wondering, would it be worth backporting this to 8.x? Would help a lot to be able to treat this as a baseline support. I can put together the port if it sounds like it could be possible.

@TimothyGu

This comment has been minimized.

Member

TimothyGu commented Apr 24, 2018

@guybedford I'd rather not since there might be code out there checking typeof URL to detect Node.js/browsers.

@guybedford

This comment has been minimized.

Contributor

guybedford commented Apr 25, 2018

@TimothyGu I've never ever seen a pattern like that for environment detection - usually it is based on Window or Document. Every case I've seen doing typeof URL is for a feature detection to use URL, which will imply a behaviour change yes, but it should be a wanted one in those cases.

Users wanting to use the URL global in Node won't be able to until Node 8 EOL, which is over a year away, so it's a shame not to be able to really use this awesome feature in published packages that want to support Node 8 +.

@mcollina

This comment has been minimized.

Member

mcollina commented Apr 25, 2018

I'm generically -1 in adding new globals in semver-minors. Code running without 'use strict' will be affected (sigh): unfortunately there are plenty of modules that do exactly that.
I think landing it in 8 will require a @nodejs/tsc vote. It might be a good thing to check anyway what our position is, if you want to move this forward and champion this.

@targos

This comment has been minimized.

Member

targos commented Apr 25, 2018

If you feel strongly about it, can you please open a new issue for that discussion?

guybedford added a commit to guybedford/node that referenced this pull request Apr 25, 2018

url: expose the WHATWG URL API globally
Install URL and URLSearchParams on the global object, like they can be
found in browser environments.

PR-URL: nodejs#18281
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@guybedford

This comment has been minimized.

Contributor

guybedford commented Apr 25, 2018

I've created #20306 to track. @mcollina what sort of code examples without 'use strict' might be affected by this do you think?

@mcollina

This comment has been minimized.

Member

mcollina commented May 2, 2018

try {
  new URL("http://google.com")
} catch (e) {
  // we are on Node 10!
  // so we can use XYZ.
}

also

if (!global.URL) {
  // assume something that's not true anymore 
}

MayaLekova added a commit to MayaLekova/node that referenced this pull request May 8, 2018

url: expose the WHATWG URL API globally
Install URL and URLSearchParams on the global object, like they can be
found in browser environments.

PR-URL: nodejs#18281
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment