Skip to content
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

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

Closed
wants to merge 1 commit into from

Conversation

guybedford
Copy link
Contributor

This backports #18281 to the v8.x branch: Install URL and URLSearchParams on the global object, like they can be found in browser environments.

What makes this feature so useful is that code written for both the web and Node can do simple URL manipulation to spec, without having to jump through environment detections etc.

But if I wanted to publish a package to npm while supporting NodeJS v8.x this isn't currently possible.

In order to enable these universal workflows, it would be great to get this v8.x support in.

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 nodejs-github-bot added lib / src Issues and PRs related to general changes in the lib or src directory. v8.x labels Apr 25, 2018
@MylesBorins MylesBorins added the semver-minor PRs that contain new features and should be released in the next minor version. label Apr 26, 2018
@MylesBorins
Copy link
Contributor

The original PR was Semver-Major, this is at the very least Semver-Minor @nodejs/lts thoughts?

@jasnell
Copy link
Member

jasnell commented Apr 26, 2018

We have to be very careful introducing new globals as there really is a non-zero risk of breaking existing code. We need to be cautious here.

@guybedford
Copy link
Contributor Author

guybedford commented Apr 26, 2018

This does seem to me one of the safer globals to add as expectations around it are all based on fairly common patterns. I know of no patterns where a typeof URL detection affects other feature detections apart from only URL usage itself. And I can't think of any cases apart from code beginning with a typeof URL or (try { new URL })` feature detection checks that would be affected at all.

The biggest risk does seem like strict mode code assigning to the URL global by mistake as @mcollina rightly pointed out here. But that is a risk not of breaking old code, but for new code expecting the URL global, and affects the current v10 release equally.

@guybedford
Copy link
Contributor Author

To reiterate my motivation here is actually to support import.meta.url in Rollup, without inadvertently creating a browserify dependency on require('url') for libraries (rollup/rollup#2164). Having this universal baseline would be very useful.

@guybedford
Copy link
Contributor Author

Ping here, would it be possible to move this forward?

@mcollina
Copy link
Member

mcollina commented May 22, 2018

This is an LTS line, and we need to be careful. Even if we add this now, It will be in 8.12, and there are a lot of Node 8 users out there that will not upgrade their developer machines (or in cloud functions). In the end your users will have to do some 'browser' field trick (I'm not super familiar with rollup) anyway to grab URL.

Anyway, here is a CITGM run

CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1425/
CITGM (master): https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1424/

@guybedford
Copy link
Contributor Author

Thanks @mcollina for the update. To play this cautiously, this could certainly be reverted if there do happen to be any issues at all.

The trick basically looks like - new (typeof URL !== 'undefined' ? URL : require('url').URL)(urlStr) here, the main issue being that browserifying the above pulls all of the url module, so it's a huge win to universal workflows if we can get this in, which we otherwise have to wait another year and a half for.

@mcollina
Copy link
Member

which we otherwise have to wait another year and a half for.

Node 10 becomes LTS in October. Am I missing some context?

I do not think this is the correct move for the Node.js ecosystem. As an example AWS Lambda runs on Node 8.10, which won't have this global. However the official Node 8 doc will show that this global is present. This creates a lot of friction in the ecosystem, as some key part of our API only works on a later version in the lts cycle.

On a side note, cannot you use https://github.com/defunctzombie/package-browser-field-spec#spec?
Or new (typeof URL !== 'undefined' ? URL : require('ur' +l').URL) the latter used to work with browserify to avoid these cases.

@mcollina mcollina added the tsc-agenda Issues and PRs to discuss during the meetings of the TSC. label May 22, 2018
@mcollina
Copy link
Member

This is really a matter for the @nodejs/tsc

@guybedford
Copy link
Contributor Author

When would Lambda update to the new 8.x branches do you think?

Node 8 LTS ends next October in terms of what environments the output needs to support. Yes new (typeof URL !== 'undefined' ? URL : require('ur'+'l').URL) does work well here actually in avoiding the browserify issue.

If you feel it is is unnecessary risk I won't push this, it's just a very nice to have from a browser workflow perspective.

@guybedford
Copy link
Contributor Author

Seems I misread the LTS schedule - Node 8.x LTS is until the end of 2019.

@jasnell
Copy link
Member

jasnell commented May 22, 2018

I'm generally -0 on this. I'm not going to block it but our policy has been to be very conservative with LTS branches. I'd rather not introduce a new global in 8.x

@guybedford
Copy link
Contributor Author

My worry here is that packages will continue to be published with users writing require('url').URL or using a third-party package like url-parse (3.5 million downloads per week) that isn't necessarily fully WhatWG compliant and adds to bundle sizes.

Note also that the standard browserify URL implementation still doesn't support require('url').URL (defunctzombie/node-url#37).

We have an opportunity here to unify workflows and significantly improve experience for users avoiding these catches as well as unnecessary URL code being pulled into bundles, with some small risk of breakage which seems quite unlikely but can be reverted in the case that it does happen.

If there are any hard -1's I can close the issue now certainly, but otherwise perhaps it is worth bringing up at the TSC meeting indeed then? I'll gladly observe / attend if you think it would be worthwhile.

@MylesBorins
Copy link
Contributor

I think this is something for @nodejs/lts to discuss first.

Regarding introducing new symbols or things not present in certain cloud providers... that is already the case for any Semver-Minor additions to the platform or Semver-Patch breaking changes to experimental apis (this just happened for http2).

We have a Semver-Minor release planned for the next 8.x release. Considering that we recently backported n-api to 6.x for similar reasons, it seems reasonable enough for us to consider this. If we don't want to land this change due to potential confusion, I think we should consider revisiting our entire Semver-Minor process for LTS (which I am not entirely opposed to).

@mcollina
Copy link
Member

I’m happy to defer to @nodejs/lts.

@fhinkel
Copy link
Member

fhinkel commented May 29, 2018

@mcollina Do you want to remove the tsc-agenda label since this goes to nodejs/lts? Thanks!

@mcollina mcollina removed the tsc-agenda Issues and PRs to discuss during the meetings of the TSC. label May 29, 2018
@guybedford
Copy link
Contributor Author

If the lts team isn't actively looking at this, perhaps it might still be best to have this discussed on TSC agenda?

@richardlau
Copy link
Member

There is a lts-agenda label, but at the moment there are no scheduled regular meetings: nodejs/Release#333

@guybedford
Copy link
Contributor Author

Any update on this at all?

@MylesBorins
Copy link
Contributor

ping @nodejs/lts @mhdawson @rvagg @jasnell @Fishrock123

@rvagg
Copy link
Member

rvagg commented Jun 14, 2018

yeah, sorry, I don't think I can support the introduction of a new global into an LTS, that's semver-major for good reason

@guybedford
Copy link
Contributor Author

Web browsers ship new globals all the time, so I'm not sure where this comes from that new globals in Node are major releases? It seems like there is some history here that I'm not aware of perhaps? Would help to know.

I think it will be incredibly sad for Node.js if Worker, URL and fetch can only be expected to be globals in Node after 2022...

@MylesBorins
Copy link
Contributor

@rvagg is that an explicit -1?

@rvagg
Copy link
Member

rvagg commented Jun 20, 2018

@MylesBorins yes, sorry, -1 from me

Web browsers ship new versions every 6 weeks, the user model is very different. We're supporting deployments that need to be stable over years, that's the whole purpose of the LTS model we have.

Current is more similar to the browser model. LTS is something entirely different with a different target audience and it's critical that we maintain stability and ensure no surprises in order to maintain the significant trust we've built up in our LTS processes amongst their userbase.

@guybedford
Copy link
Contributor Author

Sure, thanks all for consideration.

@dnalborczyk
Copy link
Contributor

Web browsers ship new versions every 6 weeks, the user model is very different. We're supporting deployments that need to be stable over years, that's the whole purpose of the LTS model we have.

I'm sorry, but browsers have the exact same goal of not breaking the web ever. I would even argue more so than node.js, as the world wide browser installations by sheer numbers for sure outweigh the installation base of node.js. on top of it, keep in mind that there's also competing engines with competing implementations in the browser world, which have a far higher risk of "breaking things".

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lib / src Issues and PRs related to general changes in the lib or src directory. semver-minor PRs that contain new features and should be released in the next minor version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants