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
Replace baseUrl
with prefixUrl
#829
Conversation
migration-guides.md
Outdated
@@ -83,6 +81,7 @@ To use streams, just call `got.stream(url, options)` or `got(url, {stream: true, | |||
- No `forever` option. You need to use [forever-agent](https://github.com/request/forever-agent). | |||
- No `proxy` option. You need to [pass a custom agent](readme.md#proxies). | |||
- No `removeRefererHeader` option. You can remove the referer header in a [`beforeRequest` hook](https://github.com/sindresorhus/got#hooksbeforeRequest): | |||
- No `baseUrl` option. Instead, there is `prefixUrl` which appends ending slash if not present. |
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.
- No `baseUrl` option. Instead, there is `prefixUrl` which appends ending slash if not present. | |
- No `baseUrl` option. Instead, there is `prefixUrl` which appends ending slash if not present. If `prefixUrl` is used, it will be always prepended to the `url` argument. |
Actually should we do
if (options.prefixUrl && !urlArgument.startsWith('http:') && !urlArgument.startsWith('https:') && !urlArgument.startsWith('unix:')) {
...
}
or let this behavior be?
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.
/cc @sholladay
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.
My opinion has always been that input
should not be able to override the host of a prefixUrl
, otherwise it's not really a prefix. If such an override is needed, and it very well may be for some use cases, then I feel that full URL resolution is the answer (I.e. baseUrl
). I don't think we have any business keeping a list of "special" schemes like http:
where absolute URLs can override the target host. That is the job of a URL resolver, and I'd prefer to either fully embrace URL resolution or not at all.
I'm personally fine with having both prefixUrl
and baseUrl
. But I know Sindre felt that would be confusing.
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.
What if we pass a URL instance to input
? I think we should ignore prefixUrl
then (same for Ky).
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.
Why does a URL instance change the behavior as opposed to a string? I would expect them to behave the same.
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.
The URL instance is an absolute URL. Is there any use case for joining two absolute URLs? The string does not need to be absolute. That's the difference.
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.
Oh, right. I forgot that URL
instances are always absolute, because that's such a silly limitation in the URL
class. So, yeah, if absolute URLs are allowed to override prefixUrl
, then it makes sense. However, in terms of the code, I would probably not explicitly check for URL
instances, if possible. After all, what if URL
starts supporting relative URLs in the future? I think they could introduce support for that without breaking too many things. And I hope they do, personally.
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.
My opinion has always been that input should not be able to override the host of a prefixUrl
Co-Authored-By: Sindre Sorhus <sindresorhus@gmail.com>
There are still some references to
|
Fixed |
Nice work! :) |
Checklist
Fixes #783
Fixes #814