-
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
URL does not match the web; does not allow changing protocol from about:
#49319
Comments
Note this bug dates back to node v10.0.0, so it'd be great to backport this back as far as is possible (to all non-EOL lines, ideally) |
Since Node switch to ada, does this bug still occur? Is it potentially an issue with the spec? |
cc @anonrig |
the bug in node seems to be in every version that has URL; from 10.0.0 onwards, so it doesn't seem switching to ada caused or fixed it. |
I doubt it's an issue with the spec, since it works fine if the original URL is http or https, and it works fine on Chrome and Firefox and Safari - meaning it's solely a bug in node's implementation. |
This might actually be an issue with the spec: the protocol setter calls basic URL parse passing it
which suggests to me that you should not be able to change the protocol from a special scheme to a non-special scheme, or conversely. But, generally speaking WHATWG specs are supposed to document reality, so if no browsers implement the behavior in the spec, I believe that would be regarded as an issue in the spec. |
aha, interesting, thanks for that context |
Some more fun quirks from browsers doing their own thing here: |
I don't think browsers are the best thing to follow on this one. Object.assign(new URL('git+file://a'), { protocol: 'file:'}).toString()
"file:///" I can't imagine updating the spec to allow THAT. |
This is 100% correct. |
True, but the entire point of node implementing URL was to follow browsers, iirc, and any deviations (even from weird behavior) contradicts the goal of increasing portable code. |
I've asked about this in the WHATWG matrix channel; will report back if anything useful comes out of it. |
What browsers follow is defined by the WHATWG specification, and they don't follow the specification (even the web-platform tests results show it). Unfortunately, we follow it strictly (and getting punished by it). Referring: https://wpt.fyi/results/url?label=experimental&label=master&aligned |
Closing the issue since this is an intended behavior of the whatwg url specification. |
@anonrig closing doesn't feel appropriate - the specification is irrelevant; the only point of having URL in node is to match browsers. |
The question here is IMO. Are we aiming for spec or web compliance? wdyt @benjamingr |
cc @nodejs/url @jasnell |
@ljharb open an issue in WHATWG to change the behavior or the spec? |
I’ll certainly follow the whatwg discussion, and it’s fine if this is low priority, but issues aren’t closed due to being low priority usually - and it seems like “we follow the spec” vs “we follow the browser” for URL is a tsc consensus thing - has that been discussed? |
I'm happy to (and it seems so is @anonrig ) to bring this up at the TSC meeting but I don't think that'd be super useful. We operate by lazy consensus. This issue was opened, several collaborators commented. None of them are in actual disagreement about Node doing what the spec says from what I can tell. We can count TSC members too but that shouldn't matter. The collaborators "own" the code and the TSC mediates technical disagreements and votes on them if it comes to that. From what I can tell unless someone who commented disagrees this isn't actionable to the TSC? (cc @Ethan-Arrowood @ronag @anonrig ) The TSC will 100% discuss it for 1-2 minutes and then say something like "we will monitor what the WHATWG effort ends up with". Unless you want to bring "let's deviate from the spec when browsers do" as an item? In that case it should probably be a separate issue. |
That all makes sense; I’m fine just leaving the issue open pending the whatwg discussion regardless. |
As for the larger question, a ton of design decisions have been made in node for the rationale “we follow browsers, so code is more portable” - including ESM’s resolution compared to that of CJS, using URLs instead of file paths, etc. If matching browsers isn’t the goal, then those decisions and URL following the spec instead of browsers are contradicting each other. |
I agree there is an inconsistency in our decision making depending what module/people are involved. |
@benjamingr This is an issue that surfaces every X amount of time which is not specific to url itself. For example, couple of months ago I had a write a blog post to use it as a reference whenever a opaque-origin related issue is created on this repository where the issuer is stating Chrome and Firefox is doing the right thing and Node.js & Safari has a bug. Ref: https://www.yagiz.co/url-parsing-and-browser-differences Personally, I think we need consensus towards what the users expect, and what the specification is. From what the documentation is saying URL class is under the section: "WHATWG URL API", but even that was not sufficient for this discussion. Therefore, I raise my concern to resolve this once in for all. PS: Browsers are trying to conform to the web-platform-tests as part of Interop 2023, and eventually either the URL specification will change or the browsers will follow-up with the specification implementation. |
@anonrig what actionable outcome do you expect from the discussion other than "we should follow the spec reasonably and consult with WinterCG when making server side specific deviations" (which isn't actionable)? |
I think that's an acceptable outcome, no? Then at least we have something to fallback to when these conversations start... |
The relevant part of the specification is this: Reference: https://url.spec.whatwg.org We find explicit test cases that come with the specification such as ...
So it is very much deliberate. In fact, there is an issue open on this very topic: |
(Note that whatwg/url#782 would obviate my use case for mutating an URL object, which would mean i don't care what this does as long as it matches browsers) |
A reasonable outcome would be to have a consensus on whether we are going to follow WHATWG URL specification (as we do right now), or try to match browser implementations, as suggested. |
I think we wouldn’t want to change to match browsers only to then see browsers change to start following the spec; I think this needs to get resolved at the WHATWG level before we do anything. I expect that WHATWG will update the spec to match browsers and then we’ll update accordingly, but who knows. |
I totally agree with that plan; it remains useful in general tho to clarify whether the thing to follow is "web reality" or "the html spec and friends". |
Out of curiosity, what's the goal of using Object.assign(new URL('file://localserver/dev/null'), {
username: 'livia',
protocol: 'https:',
}).href === 'https://localserver/dev/null';
Object.assign(new URL('file://localserver/dev/null'), {
protocol: 'https:',
username: 'livia',
}).href === 'https://livia@localserver/dev/null'; If the goal is to have WHATWG URL-compliant |
It’s just a short way to do a string of assignments from an object. |
Version
20.5.1
Platform
Darwin NAME.local 20.6.0 Darwin Kernel Version 20.6.0: Sun Nov 6 23:17:00 PST 2022; root:xnu-7195.141.46~1/RELEASE_X86_64 x86_64
Subsystem
URL
What steps will reproduce the bug?
Run
Object.assign(new URL('about:blank'), { protocol: 'http' }).protocol === 'http:'
in the REPL.How often does it reproduce? Is there a required condition?
Always.
What is the expected behavior? Why is that the expected behavior?
true
. It's expected because every browser does it, and because it works fine if the initial URL is an http/https one.What do you see instead?
false
Additional information
No response
The text was updated successfully, but these errors were encountered: