Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
withprefixUrl
#829Replace
baseUrl
withprefixUrl
#829Changes from 1 commit
38724e5
9d6f646
d65ec15
09e0b0c
8364ac7
9b32c96
0255c03
cd1b14b
4f6a5bf
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Actually should we do
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 aprefixUrl
, 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 likehttp:
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
andbaseUrl
. 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 ignoreprefixUrl
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 theURL
class. So, yeah, if absolute URLs are allowed to overrideprefixUrl
, then it makes sense. However, in terms of the code, I would probably not explicitly check forURL
instances, if possible. After all, what ifURL
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.
👍 (We can reconsider if we get a lot of complain about this)