-
-
Notifications
You must be signed in to change notification settings - Fork 366
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
Add baseUrl option, rename prefixUrl, and allow leading slashes in input #606
base: main
Are you sure you want to change the base?
Conversation
i see that it's going to be a breaking change, maybe it's worth about discusssing about the next major? i think it's good to remove default timeout and retry settings, because many people come from fetch and axios and it's not obvious ky have these defaults. |
I would be okay with removing the default timeout, but not the default retries. |
baseUrl looks like what I want. I only care about the URL's origin, and also need the ability to pass full URLs. If it works like this, it's perfect: ky.get('/api/v1/instance', { baseUrl : 'https://ditto.pub' }); // GET https://ditto.pub/api/v1/instance
ky.get('https://ditto.pub/api/v1/timelines/home?max_id=1234', { baseUrl : 'https://ditto.pub' }); // GET https://ditto.pub/api/v1/timelines/home?max_id=1234 In this case, I don't care about a path in the baseUrl. I assume it works like this: ky.get('/api/v1/instance', { baseUrl : 'https://ditto.pub/@alex' }); // GET https://ditto.pub/api/v1/instance But I will never pass a baseUrl with a path anyway. |
@alexgleason correct, each of those examples behaves as you described. I would expect most people to structure their requests like this: const api = ky.extend({ baseUrl : 'https://ditto.pub/api/v1/' });
api.get('instance'); // GET https://ditto.pub/api/v1/instance
api.get('timelines/home?max_id=1234', { baseUrl : 'https://ditto.pub/api/v1' }); // GET https://ditto.pub/api/v1/timelines/home?max_id=1234
api.get('https://example.com'); // GET https://example.com
api.get('/api/v2/secret'); // GET https://ditto.pub/api/v2/secret But you could certainly put the |
@sholladay I think the way to overcome code complexity and confusion while solving all use-cases is to just support a resolver function like mentioned here: #291 (comment) But I think it should actually look like this: const api = ky.create({
// Accept any `Input` and return any `Input`. Flexible and pure.
resolver(input: Input): Input {
// It's not actually complicated to do this yourself either.
const url = input instanceof Request ? input.url : input.toString();
return new Request(new URL(url, 'https://ditto.pub'), input);
}
}); We can keep the const api = ky.create({
prefixUrl: 'https://ditto.pub', // WARNING: `prefixUrl` is deprecated.
resolver(input: Input): Input { // ERROR: `prefixUrl` and `resolver` cannot be specified at the same time.
// ...
}
}); For improved DX, Ky can include resolvers for a prefix and baseUrl. import ky from 'ky';
const api = ky.create({
resolver: ky.prefix('https://ditto.pub/api/v1')
});
const api = ky.create({
resolver: ky.baseUrl('https://ditto.pub')
}); |
@alexgleason the way I am planning to solve that is by allowing users to return a URL instance in a |
@sholladay That could work as long as we have the |
Can it even get to the point of It's fine as a hook, but I think it might still have to be a separate hook. |
While it doesn't provide input, you can use |
@sholladay Currently we can already do that with |
What's the use case that causes you to need a base URL that is unknown at the time of creating the Ky instance? In other words, why is it dynamic? |
@sholladay Supporting multiple accounts on Mastodon. |
I ended up just writing a custom bare minimum fetch wrapper and accepting that I will have to do |
@bertho-zero you didn't explain what you prefer about this PR and why. Your regex is shorter but the current code is more readable and should have slightly better performance. Both are fine, though, and I don't have strong feelings about it. |
- After `prefixUrl` and `input` are joined, the result is resolved against the [base URL](https://developer.mozilla.org/en-US/docs/Web/API/Node/baseURI) of the page (if any). | ||
- Leading slashes in `input` are disallowed when using this option to enforce consistency and avoid confusion about how the `input` URL is handled, given that `input` will not follow the normal URL resolution rules when `prefixUrl` is being used, which changes the meaning of a leading slash. | ||
- After `startPath` and `input` are joined, the result is resolved against the [base URL](https://developer.mozilla.org/en-US/docs/Web/API/Node/baseURI) of the page (if any). | ||
- Leading slashes in `input` are disallowed when using this option to enforce consistency and avoid confusion about how the `input` URL is handled, given that `input` will not follow the normal URL resolution rules when `startPath` is being used, which changes the meaning of a leading slash. |
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.
Leading slashes in
input
are disallowed when using this option to enforce consistency and avoid confusion [...]
From what I can tell from the changed code, this is no longer the case?
I personally prefer it that way though and think that we should keep that behaviour
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.
Indeed. As mentioned in the TODOs section, I haven't gotten around to updating the docs yet, other than a simple find & replace. But thank you for the reminder. 🙂
I like #561 because I don't understand why we should throw an error if the input starts with a slash, I read the topic but still don't find this constraint justified. I don't like #606 because I can have a prefix like |
this._input = this._input.slice(1); | ||
} | ||
|
||
this._input = this._options.startPath + this._input; |
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.
I think if _input
starts with a protocol (or is a valid URL?) then we should ignore startPath
in order to avoid an URL like http://foo.com/http://bar.com
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.
That kind of "double URL" construct is useful for reverse proxies. It should be supported.
Also, you would be surprised how hard URL parsing is. Especially for a library like Ky that supports non-browsers.
You don't need to split it up like that, you can just put the whole thing in |
This PR aims to improve the flexibility of input URLs and options for resolving them prior to the request.
prefixUrl
is renamed tostartPath
input
before any other processing takes placestartPath
can still be a full URL if needed, however the name is meant to signal its primary use case is a simple base path without a domain, e.g./api
baseUrl
option is addedinput
(including anystartPath
) is resolved against thebaseUrl
according to standard URL resolution rulesbaseUrl
will respect the HTML<base>
tag if present, and is similar to it, except that it only applies to Kyinput
can now be any valid URL, with or without a leading slashstartPath
baseUrl
Possible names for the string that is prepended to
input
startPath
(as is currently implemented here)pathPrefix
prefixPath
rootPath
TODO:
startPath
option