-
Notifications
You must be signed in to change notification settings - Fork 29.7k
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: Fix resolving from non-file to file URLs. #1277
Conversation
@@ -495,7 +495,7 @@ Url.prototype.resolveObject = function(relative) { | |||
} | |||
|
|||
result.protocol = relative.protocol; | |||
if (!relative.host && !hostlessProtocol[relative.protocol]) { | |||
if (!relative.host && !/file:?/.test(relative.protocol) && !hostlessProtocol[relative.protocol]) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't the regex match the whole string?
Style: long line, please wrap at 80 columns (one clause per line.)
When resolving a reference URL with the 'file' scheme an no host against a base URL without the 'file' scheme, the first path element of the reference URL is used as the host for the target URL. This results in an invalid target URL. This change makes an exception for file URLs so that the host is not mangled during URL resolution.
Thanks for the feedback. I believe I have addressed the style concerns. Is there anything else that I need to do? |
I'm tagging this semver-minor because I expect that's what it is. Someone else should do the final sign-off and land it. |
Ping @iojs/collaborators. |
@petkaantonov are you saying the current output is correct/more desireable in your comment above? |
@silverwind No I am saying the new output is correct, on top of that I am saying that current output is almost always incorrect in whatwg tests |
In that case, LGTM |
@bnoordhuis Shouldn't this either be a patch or a major? I don't see how it could be a minor |
@petkaantonov I guess it's not. I'll remove the label. |
When resolving a reference URL with the 'file' scheme an no host against a base URL without the 'file' scheme, the first path element of the reference URL is used as the host for the target URL. This results in an invalid target URL. This change makes an exception for file URLs so that the host is not mangled during URL resolution. PR-URL: #1277 Reviewed-By: Roman Reiss <me@silverwind.io> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Petka Antonov <petka_antonov@hotmail.com>
landed in 1e94057 |
Cool, thanks! |
When resolving a reference URL with the 'file' scheme an no host
against a base URL without the 'file' scheme, the first path element
of the reference URL is used as the host for the target URL. This
results in an invalid target URL.
This change makes an exception for file URLs so that the host is not
mangled during URL resolution.