url: expose the WHATWG URL API globally #18281

targos · 2018-01-21T16:18:02Z

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 · 2018-01-21T16:31:35Z

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

targos · 2018-01-21T16:33:36Z

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 · 2018-01-21T16:46:16Z

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

apapirovski

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 · 2018-01-21T18:25:24Z

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

mcollina · 2018-01-21T20:38:44Z

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.}

evanlucas · 2018-01-22T14:43:16Z

doc/api/url.md

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


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

evanlucas · 2018-01-22T14:43:21Z

doc/api/url.md

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


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

thanks. updated

targos · 2018-01-22T14:48:50Z

CI: https://ci.nodejs.org/job/node-test-pull-request/12656/

jasnell

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

ChALkeR · 2018-01-22T16:31:35Z

-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.

silverwind · 2018-01-22T20:14:53Z

lib/internal/bootstrap_node.js

@@ -327,6 +328,24 @@
    setupInspector(originalConsole, wrappedConsole, Module);
  }

+  function setupGlobalURL() {
+    const { URL, URLSearchParams } = NativeModule.require('internal/url');


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

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

node/lib/internal/bootstrap_node.js

Lines 80 to 82 in a3555d0

// Ensure setURLConstructor() is called before the native

// URL::ToObject() method is used.

NativeModule.require('internal/url');

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>

targos · 2018-01-29T09:26:23Z

New CI: https://ci.nodejs.org/job/node-test-pull-request/12797/
Edit: AIX rebuild: https://ci.nodejs.org/job/node-test-commit-aix/12156/

targos · 2018-01-30T08:31:25Z

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 · 2018-01-30T21:26:03Z

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 · 2018-01-31T14:58:17Z

@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 · 2018-01-31T15:08:48Z

Landed in 3124146

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>

fregante · 2018-02-02T11:35:51Z

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.

guybedford · 2018-04-24T20:16:23Z

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 · 2018-04-24T21:25:58Z

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

guybedford · 2018-04-25T11:46:01Z

@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 · 2018-04-25T19:56:44Z

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 · 2018-04-25T20:04:35Z

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

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 · 2018-04-25T21:29:21Z

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

mcollina · 2018-05-02T15:17:46Z

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 
}

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>

nodejs-github-bot added the lib / src Issues and PRs related to general changes in the lib or src directory. label Jan 21, 2018

targos added whatwg-url Issues and PRs related to the WHATWG URL implementation. and removed lib / src Issues and PRs related to general changes in the lib or src directory. labels Jan 21, 2018

targos added the semver-major PRs that contain breaking changes and should be released in the next major version. label Jan 21, 2018

apapirovski approved these changes Jan 21, 2018

View reviewed changes

addaleax approved these changes Jan 21, 2018

View reviewed changes

watilde approved these changes Jan 21, 2018

View reviewed changes

TimothyGu approved these changes Jan 21, 2018

View reviewed changes

BridgeAR approved these changes Jan 22, 2018

View reviewed changes

joyeecheung approved these changes Jan 22, 2018

View reviewed changes

bnoordhuis approved these changes Jan 22, 2018

View reviewed changes

evanlucas approved these changes Jan 22, 2018

View reviewed changes

targos force-pushed the global-url branch from 68c5b6a to 61cda62 Compare January 22, 2018 14:48

jasnell approved these changes Jan 22, 2018

View reviewed changes

silverwind reviewed Jan 22, 2018

View reviewed changes

maclover7 force-pushed the master branch from bb5575a to 993b716 Compare January 26, 2018 22:02

cjihrig force-pushed the master branch from 993b716 to 082f952 Compare January 26, 2018 22:36

targos force-pushed the global-url branch from 61cda62 to b7693a7 Compare January 29, 2018 09:23

targos added the notable-change PRs with changes that should be highlighted in changelogs. label Jan 31, 2018

targos closed this Jan 31, 2018

targos deleted the global-url branch January 31, 2018 15:08

jdalton mentioned this pull request Mar 15, 2018

module: add createRequireFunction method #19360

Closed

4 tasks

thom4parisot mentioned this pull request Apr 24, 2018

The WHATWG URL API is now a global. thom4parisot/nodebook#271

Closed

guybedford mentioned this pull request Apr 25, 2018

[v8.x backport] url: expose the WHATWG URL API globally #20306

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

url: expose the WHATWG URL API globally #18281

url: expose the WHATWG URL API globally #18281

targos commented Jan 21, 2018 •

edited

vkurchatkin commented Jan 21, 2018

targos commented Jan 21, 2018

cjihrig commented Jan 21, 2018

apapirovski left a comment •

edited

addaleax left a comment

targos commented Jan 21, 2018

mcollina commented Jan 21, 2018

TimothyGu left a comment

bnoordhuis left a comment

evanlucas Jan 22, 2018

evanlucas Jan 22, 2018

targos Jan 22, 2018

targos commented Jan 22, 2018

jasnell left a comment

ChALkeR commented Jan 22, 2018 •

edited

silverwind Jan 22, 2018

TimothyGu Jan 22, 2018

targos commented Jan 29, 2018 •

edited

targos commented Jan 30, 2018

Trott commented Jan 30, 2018

mhdawson commented Jan 31, 2018

targos commented Jan 31, 2018

fregante commented Feb 2, 2018

guybedford commented Apr 24, 2018

TimothyGu commented Apr 24, 2018

guybedford commented Apr 25, 2018

mcollina commented Apr 25, 2018

targos commented Apr 25, 2018

guybedford commented Apr 25, 2018

mcollina commented May 2, 2018

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

url: expose the WHATWG URL API globally #18281

url: expose the WHATWG URL API globally #18281

Conversation

targos commented Jan 21, 2018 • edited

Checklist

Affected core subsystem(s)

vkurchatkin commented Jan 21, 2018

targos commented Jan 21, 2018

cjihrig commented Jan 21, 2018

apapirovski left a comment • edited

Choose a reason for hiding this comment

addaleax left a comment

Choose a reason for hiding this comment

targos commented Jan 21, 2018

mcollina commented Jan 21, 2018

TimothyGu left a comment

Choose a reason for hiding this comment

bnoordhuis left a comment

Choose a reason for hiding this comment

evanlucas Jan 22, 2018

Choose a reason for hiding this comment

evanlucas Jan 22, 2018

Choose a reason for hiding this comment

targos Jan 22, 2018

Choose a reason for hiding this comment

targos commented Jan 22, 2018

jasnell left a comment

Choose a reason for hiding this comment

ChALkeR commented Jan 22, 2018 • edited

silverwind Jan 22, 2018

Choose a reason for hiding this comment

TimothyGu Jan 22, 2018

Choose a reason for hiding this comment

targos commented Jan 29, 2018 • edited

targos commented Jan 30, 2018

Trott commented Jan 30, 2018

mhdawson commented Jan 31, 2018

targos commented Jan 31, 2018

fregante commented Feb 2, 2018

guybedford commented Apr 24, 2018

TimothyGu commented Apr 24, 2018

guybedford commented Apr 25, 2018

mcollina commented Apr 25, 2018

targos commented Apr 25, 2018

guybedford commented Apr 25, 2018

mcollina commented May 2, 2018

targos commented Jan 21, 2018 •

edited

apapirovski left a comment •

edited

ChALkeR commented Jan 22, 2018 •

edited

targos commented Jan 29, 2018 •

edited