-
Notifications
You must be signed in to change notification settings - Fork 29.8k
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
Comments
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. |
Tagging tsc-agenda @nodejs/tsc |
It seems premature to me to have the discussion brought up to the TSC before it happened here. |
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. |
Related discussion to relative URL in WHATWG: whatwg/url#421 |
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? |
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 Also, keep in mind that the deprecation of |
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. |
Some ideas.
|
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 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 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) |
Someone else may want to take this up. |
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 |
@jasnell I see two ways
PS. Or better yet we can deprecate Which one is better approach? I have time this weekend so I can implement it. |
|
@Fishrock123 I will take it. Will see how it goes. |
Was this ever resolved? Does WHATWG URL API not offer a good replacement for the |
Not to my knowledge, and it seems unlikely that it ever will, because WHATWG has never really cared about Node. |
Signed-off-by: James M Snell <[email protected]> Fixes: nodejs#25099
Signed-off-by: James M Snell <[email protected]> Fixes: nodejs#25099
That does not solve this issue. 🤦 The UX is still terrible for |
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 |
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. |
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 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:
Anything on I personally lean towards strictness, so my ideal would be something more checked than the above snippet. Implementor's choice, really. |
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 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? |
Anything that is not this:
Something like "any form of |
Perhaps we could add a more generic |
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 |
This is not concerned with the exact semantics of |
@Fishrock123 is the expectation of general usage that it always outputs a valid URL (including the protocol)? |
Ok I'll tease the issue apart more, there are two issues:
|
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}`);
console.log(u.href); Is this just a documentation issue then? |
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. |
Signed-off-by: James M Snell <[email protected]> Fixes: #25099 PR-URL: #37784 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Vladimir de Turckheim <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Beth Griggs <[email protected]>
Signed-off-by: James M Snell <[email protected]> Fixes: #25099 PR-URL: #37784 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Vladimir de Turckheim <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Beth Griggs <[email protected]>
If this is documented appropriately and if the old url api is no longer deprecated, feel free to close this. |
A fully adaptable one-liner replacer for URL.from = (urlObject) => String(Object.assign(new URL("http://a.org"), urlObject)) Test it here |
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. |
@jasnell it makes perfect sense, yes, please do kindly add a method to the node url module |
another thing I'd like to point out is that 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 |
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:
I do not agree that deprecating
url.format()
with no such API alternative is acceptable.The deprecation was done in #22715
The text was updated successfully, but these errors were encountered: