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

WhatWG URL provides no suitable replacement for url.format() #25099

Closed
Fishrock123 opened this issue Dec 17, 2018 · 41 comments
Closed

WhatWG URL provides no suitable replacement for url.format() #25099

Fishrock123 opened this issue Dec 17, 2018 · 41 comments
Labels
url Issues and PRs related to the legacy built-in url module. whatwg-url Issues and PRs related to the WHATWG URL implementation.

Comments

@Fishrock123
Copy link
Member

Fishrock123 commented Dec 17, 2018

Users have used url.format() for as long as I can remember. It is useful, reasonably ergonomic, has has existed for public use for over 7 years...

The WhatWG url alternative is absolutely not ergonomic or obvious:

String(Object.assign(new URL('<a url>'), {pathname: '/foo'}))

I do not agree that deprecating url.format() with no such API alternative is acceptable.
The deprecation was done in #22715

@Fishrock123 Fishrock123 added url Issues and PRs related to the legacy built-in url module. whatwg-url Issues and PRs related to the WHATWG URL implementation. labels Dec 17, 2018
@devsnek
Copy link
Member

devsnek commented Dec 18, 2018

fwiw, URL purposely doesn't expose ways to create relative urls, so there isn't really any way to represent the example you posted with a URL. i'd be interested in a version of the api which builds absolute urls though.

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

mcollina commented Dec 18, 2018

Tagging tsc-agenda @nodejs/tsc

@targos
Copy link
Member

targos commented Dec 18, 2018

It seems premature to me to have the discussion brought up to the TSC before it happened here.

@jasnell
Copy link
Member

jasnell commented Dec 18, 2018

Yeah, this really isn't that big of an issue. It's trivial to wrap the non-eronomic bits into a utility function that is easier to use. Alternatively, we could go ask the WHATWG for a new function.

@mcollina mcollina removed the tsc-agenda Issues and PRs to discuss during the meetings of the TSC. label Dec 18, 2018
@watilde
Copy link
Member

watilde commented Dec 18, 2018

Related discussion to relative URL in WHATWG: whatwg/url#421

@jdalton
Copy link
Member

jdalton commented Dec 20, 2018

I do not agree that deprecating url.format() with no such API alternative is acceptable.
The deprecation was done in #22715

I'm also not a fan of deprecating useful APIs with no alternatives. Is there a way to better prevent this kind of thing in the future?

@jasnell
Copy link
Member

jasnell commented Dec 20, 2018

The idea that there is no alternative in this case is incorrect. There is an alternative, it's just not as simple to use out of the box (although, it's relatively trivial to build a utility wrapper around that can sit out in userland) and some folks are unhappy with the alternative. Also, it would be relatively straightforward to submit a proposal to add a URL.format() utility function to the standard if someone wished to do so.

Also, keep in mind that the deprecation of url.format() is docs only and is expected to remain docs-only for quite some time.

@jdalton
Copy link
Member

jdalton commented Dec 20, 2018

The idea that there is no alternative in this case is incorrect. There is an alternative, it's just not as simple to use out of the box

I should have clarified. I'm meant no core API alternatives. I see this happen in Node when deprecating/removing chunks of functionality. Occasionally useful APIs get caught up in the cleanup. Is there a way to better prevent this kind of thing in the future?

@juanarbol
Copy link
Member

juanarbol commented Dec 29, 2018

I should have clarified. I'm meant no core API alternatives. I see this happen in Node when deprecating/removing chunks of functionality. Occasionally useful APIs get caught up in the cleanup. Is there a way to better prevent this kind of thing in the future?

I don't think so, being useful always depends on the implementation or requirement.

@jdalton
Copy link
Member

jdalton commented Dec 29, 2018

Some ideas.

  • API review board for any added or removed APIs
  • Require usage estimates and roadmapped migration plans for removed APIs

@Trott
Copy link
Member

Trott commented Dec 29, 2018

The following suggestion would be big effort, might not be possible, but if we're brainstorming and it's all about idea quantity and not (yet) about quality, here it is: Get CITGM to the point where it's green on master. Run it nightly. Back out changes that break things and fix the issue in the ecosystem, then re-land on master.

However, that will only impact runtime deprecations and breaking changes. Issues with doc-only deprecations obviously wouldn't be caught by that. But is that really that much of a problem? Undoing a doc-only deprecation is pretty simple (minus the politics, which I guess is kinda the point).

@Fishrock123 Fishrock123 self-assigned this Jan 24, 2019
@viktor-ku
Copy link
Contributor

viktor-ku commented Jun 21, 2019

@Fishrock123 can I get an update on this issue? Can I somehow help with this to bring it to node? I really want to have non-legacy alternative to url.format as well.

I understand that it is, perhaps, not in the standard, but Node can have some sort of the helper function, no? I mean it is so commonly used that it would be the shame to require exactly the same package in every new project (If node.js was to dump this whole idea in favor of 3rd party package)

@Fishrock123 Fishrock123 removed their assignment Jun 26, 2019
@Fishrock123
Copy link
Member Author

Someone else may want to take this up.

@jasnell
Copy link
Member

jasnell commented Jun 26, 2019

So I spoke a bit with Domenic Denicola about this and the relative URL issue at jsonconfeu. His suggestion was that extension APIs around WHATWG URL parser are perfectly fine.. That is, if we feel we absolutely need relative URL parsing and the equivalent of url.format, then we should feel free to add those. In the instance of url.format(), we could update the implementation to be consistent with the WHATWG URL specification.

@viktor-ku
Copy link
Contributor

viktor-ku commented Jun 27, 2019

@jasnell I see two ways

  1. Modify url.format so that it would take an object similar to what new URL() produces and also it would return a URL compatible string. Wouldn't it be a breaking change?
  2. Or we can add format method to URL itself, so it will be accessible with URL.format. But this approach might lead to confusion between url.format and URL.format.

PS. Or better yet we can deprecate url.format and add URL.from to replace that functionality. Similar to what Array.from does. So instead of new URL(url.format({})) one would do URL.from({}) which is simpler.

Which one is better approach? I have time this weekend so I can implement it.

@Fishrock123
Copy link
Member Author

URL.from({}) Sounds reasonable to me.

@viktor-ku
Copy link
Contributor

@Fishrock123 I will take it. Will see how it goes.

@torressam333
Copy link

Was this ever resolved? Does WHATWG URL API not offer a good replacement for the url.format()?

@Fishrock123
Copy link
Member Author

Not to my knowledge, and it seems unlikely that it ever will, because WHATWG has never really cared about Node.

@jasnell
Copy link
Member

jasnell commented Mar 19, 2021

#37784 changes the status of the Legacy URL apis to "Legacy" from "Deprecated"... this new status indicates that users should not use it but it's likely to never actually be removed. Once #37784 lands this issue should be resolved.

jasnell added a commit to jasnell/node that referenced this issue Mar 19, 2021
Signed-off-by: James M Snell <jasnell@gmail.com>
Fixes: nodejs#25099
jasnell added a commit to jasnell/node that referenced this issue Mar 19, 2021
Signed-off-by: James M Snell <jasnell@gmail.com>
Fixes: nodejs#25099
@Fishrock123
Copy link
Member Author

Fishrock123 commented Mar 19, 2021

That does not solve this issue. 🤦

The UX is still terrible for new URL()s. It needs an equivalent.
I don't care if Node.js does it or WhatWG does it. This is ridiculous.

@Fishrock123 Fishrock123 reopened this Mar 19, 2021
@jasnell
Copy link
Member

jasnell commented Mar 19, 2021

What is the proposal for moving forward then? What would solve this issue?

@torressam333
Copy link

What is the proposal for moving forward then? What would solve this issue?

I think we should collaborate and provide the code necessary to replace the url.format() method. That could be taking the same functionality and reimplementing it. I guess it comes down to WHY this was removed/plan to be removed in the first place?

@jasnell
Copy link
Member

jasnell commented Mar 19, 2021

WHY this was removed/plan to be removed in the first place?

To be clear, there was never a plan, or intent, to remove it.

And PRs are always welcome. There's never been a concrete proposal (by way of PR) brought forward on what to do here.

@Fishrock123
Copy link
Member Author

Fishrock123 commented Mar 19, 2021

Why didn't WhatWG think about this? I don't know. Maybe they don't care? That's my best guess at this point. I am not sorry.

I am not going to waste time making an argument about why url.format() is useful. It is, there is tons of code using it.

Why would would you want it on the new URL api? Because it acts differently, and being able to round-trip URLs can be useful.

From my original post in this thread:

The WhatWG url alternative is absolutely not ergonomic or obvious:

String(Object.assign(new URL('<a url>'), {pathname: '/foo'}))

Anything on URL itself that has either functionality like the above snippet or url.format() would probably be reasonable.

I personally lean towards strictness, so my ideal would be something more checked than the above snippet.

Implementor's choice, really.

@jasnell
Copy link
Member

jasnell commented Mar 19, 2021

Why didn't WhatWG think about this? I don't know. Maybe they don't care? That's my best guess at this point. I am not sorry.

I don't know either. We don't control the WHATWG URL standard here so the best place to have that conversation is https://github.com/whatwg/url.

If the suggestion is to add an equivalent URL.from(), we could do that but I get yelled at every time I suggest adding new methods to standard APIs so that would be another conversation for https://github.com/whatwg/url.

We just need someone willing to do that work and I'll be happy to +1 an implementation here.

More importantly tho... what is the goal post for closing this issue?

@Fishrock123
Copy link
Member Author

More importantly tho... what is the goal post for closing this issue?

Anything that is not this:

The WhatWG url alternative is absolutely not ergonomic or obvious:

String(Object.assign(new URL('<a url>'), {pathname: '/foo'}))

Something like "any form of URL.from({})", to be more clear, although I don't care if the function is called from or not.

@bmeck
Copy link
Member

bmeck commented Mar 19, 2021

Perhaps we could add a more generic from() to url itself like we have for to/from path and file urls? Likely it does need a sane way to base itself since various URLs like file: and https: are hierarchical.

@jasnell
Copy link
Member

jasnell commented Mar 19, 2021

The following is the WhatWG alternative that makes more sense in the example I think. It's certainly a lot clearer.

const u = new URL('https://example.org');
u.pathname = '/foo';
url.format(u);

//

console.log(url.href);

I just want to clarify. Is the concern more about the fact that url.format() allows for partially formed, relative URLs? e.g. url.format({ pathname: '/foo'})

@Fishrock123
Copy link
Member Author

This is not concerned with the exact semantics of url.format() but rather the general things it is used for.

@bmeck
Copy link
Member

bmeck commented Mar 19, 2021

@Fishrock123 is the expectation of general usage that it always outputs a valid URL (including the protocol)?

@Fishrock123
Copy link
Member Author

Ok I'll tease the issue apart more, there are two issues:

  1. The larger issue, that building a URL object from parts is not straightforward.
  2. Getting the string from a URL object is non-obvious. This is probably just documentation if anything. Maybe it's better than in 2018.

@jasnell
Copy link
Member

jasnell commented Mar 19, 2021

The larger issue, that building a URL object from parts is not straightforward.

const u = new URL('http://example.org');
u.host = 'example.org';
u.port = 80;
u.pathname = '/a/b/c';
u.search = '?d=e';
u.hash = '#fgh';

Alternatively...

const host = 'example.org';
const port = 80;
const pathname = '/a/b/c';
const search = '?d=e';
const hash = '#fgh';
const u = new URL(`https://${host}:${port}${pathname}${search}${hash}`);

Getting the string from a URL object is non-obvious...

console.log(u.href);

Is this just a documentation issue then?

@jasnell
Copy link
Member

jasnell commented Mar 22, 2021

Examples showing how to use the WHAT-WG URL parser to build a URL from components and how to get the serialized URL string have been added to the documentation.

ruyadorno pushed a commit that referenced this issue Mar 24, 2021
Signed-off-by: James M Snell <jasnell@gmail.com>
Fixes: #25099

PR-URL: #37784
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Vladimir de Turckheim <vlad2t@hotmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
Reviewed-By: Beth Griggs <bgriggs@redhat.com>
targos pushed a commit that referenced this issue May 1, 2021
Signed-off-by: James M Snell <jasnell@gmail.com>
Fixes: #25099

PR-URL: #37784
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Vladimir de Turckheim <vlad2t@hotmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
Reviewed-By: Beth Griggs <bgriggs@redhat.com>
@Fishrock123
Copy link
Member Author

If this is documented appropriately and if the old url api is no longer deprecated, feel free to close this.

@jfoclpf
Copy link

jfoclpf commented Dec 12, 2021

A fully adaptable one-liner replacer for url.format(urlObject) would be

URL.from = (urlObject) => String(Object.assign(new URL("http://a.org"), urlObject))

Test it here

@jasnell
Copy link
Member

jasnell commented Dec 12, 2021

We can't attach a new static member to the URL class itself unless it's added to the spec. But adding something like this to the url module would be ok.

@jfoclpf
Copy link

jfoclpf commented Dec 13, 2021

@jasnell it makes perfect sense, yes, please do kindly add a method to the node url module

@clhuang
Copy link

clhuang commented Sep 16, 2022

another thing I'd like to point out is that url.format handles ipv6 hosts correctly, whereas none of the other alternatives do:

const obj = { hostname: '2001:db8::1', port: 8080, protocol: 'http' };

url.format(obj) === 'http://[2001:db8::1]:8080'; // correct
String(Object.assign(new URL("http://a.org"), obj)) === 'http://a.org:8080/'; // incorrect
`${obj.protocol}://${obj.host}:${obj.port}` === 'http://2001:db8::1:8080'; //incorrect

@nodejs nodejs deleted a comment from vinniefalco Oct 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
url Issues and PRs related to the legacy built-in url module. whatwg-url Issues and PRs related to the WHATWG URL implementation.
Projects
None yet
Development

No branches or pull requests