Skip to content
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

prefixUrl is unneededly applied to absolute urls #291

Open
ux-engineer opened this issue Oct 21, 2020 · 25 comments · May be fixed by #606
Open

prefixUrl is unneededly applied to absolute urls #291

ux-engineer opened this issue Oct 21, 2020 · 25 comments · May be fixed by #606

Comments

@ux-engineer
Copy link

I'm having a problem with prefixUrl option:

const ky = Ky.extend({
  prefixUrl: API_BASE_URL,
});

Our backend API is returning absolute download paths which already include the base url. In those cases we end up with request urls having prefixed twice with the base url.

Could we have prefixUrl option's behavior altered so that it checks if the url already starts with this value, and if so it would not prefix it there?

I tried to create a beforeRequest hook to modify request.url, but it seems the hook cannot modify that property. That approach would have averted having to check and modify urls in possibly multiple parts of the codebase.

@sholladay
Copy link
Collaborator

sholladay commented Oct 22, 2020

We could, but my concern is it will then be confusing if it still applies the prefix to input URLs like ../meow.jpg, which is another case you probably don't want it. More likely, you want some type of URL resolution, and Ky currently does not provide any mechanism for that when using prefixUrl. It's just a simple prefix.

In the past, I've proposed adding a baseUrl option that would be akin to the HTML <base> tag, which would resolve the input URL against a base URL. Sindre thought it would be confusing to have both options, and it turns out a lot of people wanted the prefix behavior so that /meow.jpg and meow.jpg with a prefix would load the same URL, whereas those are very different URLs when resolving against a base. So that's how we ended up with the current system.

You're probably thinking this makes sense but is still a bit kludgy and I agree. One thought I've been kicking around is to let beforeRequest return a URL instance. Internally, it's a little tricky to update the request URL without messing up the request object, unfortunately. But it should be doable.

Open to other suggestions!

@haggen
Copy link

haggen commented May 7, 2021

@ux-engineer @sholladay What if we keep the current behavior of prefixUrl, but also add support for a new option resolveUrl value, to which we delegate the URL resolution process? This would helpful to me as well. I was thinking something of the sort:

const api = ky.extend({
  prefixUrl: "https://api",

  resolveUrl(url, prefix) {
    return url.startsWith("http") ? url : prefix + url;
  }
});

api.get("foo"); //=> GET https://api/foo
api.get("http://bar"); //=> GET https://bar

@sholladay
Copy link
Collaborator

A resolver function is a decent idea, although to me this is just a more verbose way to invoke the browser's URL resolution logic, compared to a baseUrl option.

@sindresorhus what if we add baseUrl but make it mutually exclusive with prefixUrl?

@sindresorhus
Copy link
Owner

If we were to add baseUrl:

  1. How would we handle usage in Node.js? For example, with ky-universal?
  2. We would have to have very good docs that clearly show the user the difference between the options and the upsides/downsides of each.

@sindresorhus
Copy link
Owner

Are there any alternatives that we haven't thought of yet? For example, could we add an option that would would make prefixUrl just pass absolute URLs through?

@sholladay
Copy link
Collaborator

How would we handle usage in Node.js? For example, with ky-universal?

The implementation would essentially do new URL(input, new URL(options.baseUrl, document?.baseURI)) (plus handling of edge cases). Should work fine, with the usual caveat that the value of baseUrl would need to be an absolute URL when used in Node, since Node doesn't have window.location or any other absolute context of its own.

Both of the below would fetch https://foo.com/a/b, in browsers as well as Node:

ky('b', {baseUrl: 'https://foo.com/a/'});
ky('https://foo.com/a/b', {baseUrl: 'https://other.com'})

We would have to have very good docs that clearly show the user the difference between the options and the upsides/downsides of each.

Agreed. I think the docs should recommend baseUrl for general use, as it aligns with browser behavior and it naturally allows input to be an absolute URL. And then it should be pointed out that ky('/b', {baseUrl: 'https://foo.com/a/'}) fetches https://foo.com/b rather than https://foo.com/a/b due to the leading slash, and mention that prefixUrl can be used instead if the latter is desired.

Are there any alternatives that we haven't thought of yet? For example, could we add an option that would would make prefixUrl just pass absolute URLs through?

Historically it's been a bit hard to determine if a URL is absolute or not in a clean and cross-platform way. That said, I recently came up with this trick, which I don't entirely hate...

const isAbsoluteUrl = (url) => {
    return (new URL(url, 'invalid:/')).protocol !== 'invalid:';
};

I suppose we could also borrow the implementation from is-absolute-url. So yeah, it's definitely possible to have special behavior for absolute URLs.

@szmarczak
Copy link
Collaborator

Initially we moved from baseUrl to prefixUrl due to ambiguity. I like the idea of resolver function as well.

Also see #146

@sholladay
Copy link
Collaborator

sholladay commented May 16, 2021

These days, the way I see it is that people in a situation like sindresorhus/got#783 should use an input of ./reports:batchGet instead of /reports:batchGet or reports:batchGet. And I feel we only need prefixUrl for people who can't/won't make that change for whatever reason... baseUrl has better support for every other use case.

Behavior baseUrl prefixUrl
Ignored when input is absolute ✅ Yes ❌ No
input of .. fetches parent directory ✅ Yes ❌ No
input of / fetches domain root ✅ Yes ❌ No

I'm still glad that we made the change to prefixUrl, as we were having a lot of trouble with the new URL() constructor and relying on window.location/document.baseURI. The simplest way I could think of at the time to give most people what they wanted without any bugs was to just join a prefix with the input.

But since then, I have figured out a simple and reliable way to do URL resolution without those problems...

const resolveUrl = (from, to) => {
    const url = new URL(to || '', new URL(from || '', 'invalid:/'));
    return url.protocol === 'invalid:' ? url.pathname + url.search + url.hash : url.href;
};

If we accept that sindresorhus/got#783 is a bit unusual, then maybe we could just have an option specific to that. So, we'd have baseUrl for most use cases and then instead of prefixUrl, we'd have dotLeadingSlash: true, which would effectively coerce / to ./.

@szmarczak
Copy link
Collaborator

should use an input of ./reports:batchGet instead of /reports:batchGet.

At Got we don't allow leading slashes. Just reports:batchGet.

we'd have dotLeadingSlash: true, which would effectively coerce / to ./.

This might create confusion. In my opinion, the lesser options to modify the URL, the better. Or this could be done in the resolveUrl function.

I have figured out a simple and reliable way to do URL resolution without those problems...

How is it different from the below?

if (prefixUrl) {
    return new URL(input, prefixUrl);
}

return new URL(input);

@sholladay
Copy link
Collaborator

At Got we don't allow leading slashes. Just reports:batchGet

Currently, Ky also enforces a lack of a leading slash in input when using prefixUrl. I've come to see that as a pain point that doesn't provide much value. If we were to keep prefixUrl, then I'd rather normalize and join the prefix than enforce a particular style. Some users just want to share an endpoints.js file with constants for each of their API paths between their backend and frontend. Those paths generally have a leading slash. We should make that easier.

How is it different from the below?

The way you've written it, prefixUrl can't be a relative URL like /api/, for example. That limitation doesn't matter for Got since Got will only be able to work with absolute prefix/base URLs anyway, but in Ky that limitation would just be frustrating for no reason.

@szmarczak
Copy link
Collaborator

Good point! I'm +1 on the baseUrl + resolveUrl solution then.

@sindresorhus
Copy link
Owner

I think we should first improve prefixUrl and then add baseUrl if it still makes sense.

It seems we can do the following to make prefixUrl easier to use:

  1. Allow / prefix, making it join the parts with / instead of just concatenation.
  2. Allow absolute URLs to pass through.

Then they will only have two differences (/ and ..), which will be easier to document.

One concern with having two options is that if someone makes a custom Got instance, it's ambiguous at the callsite which options method is used.

@szmarczak
Copy link
Collaborator

Allow absolute URLs to pass through.

👍

Allow / prefix, making it join the parts with / instead of just concatenation.

Given ky('/bar', {prefixUrl: 'https://example.com/foo/'}) do you mean:

  1. https://example.com/bar or
  2. https://example.com/foo//bar

@haggen
Copy link

haggen commented May 27, 2021

@szmarczak None. I would expect a well formed, prefixed URL: https://example.com/foo/bar

@sholladay
Copy link
Collaborator

I agree with @haggen.

@haggen
Copy link

haggen commented Jun 4, 2021

I'm currently working on my own fetch wrapper¹, probably because I suffer from severe NIH syndrome, but mostly because none of the options I've seen fully satisfied me and I came up with a URL resolution that felt powerful and intuitive at least for me. It might work for ky as well, so here's what I thought.

In my own lib I don't discriminate prefixUrl or baseUrl it's just url. You always provide one when making a request but you can extend new instances with a default URL set. The resolution algorithm is:

  1. If the URL given to the request function is fully qualified (i.e. includes hostname), nevermind the default URL and use that. e.g. If the default URL is https://google.com and you request for https://bing.com the final URL will be https://bing.com.
  2. If the URL given to the request function is an absolute path, replace the path of the default URL with that. e.g. If the default URL is https://blog.com/tags and you request for /posts the final URL will be https://blog.com/posts.
  3. If the URL given to the request function doesn't start with a slash join the default URL with that. e.g. If the default URL is https://api.com/companies/acme-inc and you request for settings/general the final URL will be https://api.com/companies/acme-inc/settings/general.

Finally you can always use a hook to modify this behavior.

What y'all think?

  1. If it's okay I could link it here.

@szmarczak
Copy link
Collaborator

@haggen It's ok to link the URL resolution code in your wrapper for comparison purposes. If you mean promotion, then Ky's issue tracker is not the right place.

Your proposal is the same as baseUrl from #291 (comment)

@haggen
Copy link

haggen commented Jun 5, 2021

@szmarczak Oh thanks my bad. First time I read it it wasn't clear to me but now it makes sense. Then I guess I agree with the baseUrl option.

@sysmat
Copy link

sysmat commented Jul 30, 2024

when I use

const apiUrl = '/api' as const;

export const API: KyInstance = ky.create({
  prefixUrl: apiUrl,
  headers: {
...
  },
});
  • but when deploy on server with context 'myapp', ky is doing absolute path request
https://sfinga2.arnes.si/api/ciscoStatus

but it should do
https://sfinga2.arnes.si/myapp/api/ciscoStatus

@sholladay
Copy link
Collaborator

Change /api to api.

Why? URLs that start with a slash are origin-relative. Meaning, they replace the entire path rather than add to it.

@sysmat
Copy link

sysmat commented Jul 30, 2024

@sholladay thx, I will try. It is vite solidjs spa, and maybe I must conf vite build

@sysmat
Copy link

sysmat commented Jul 31, 2024

@sholladay not working on vite proxy, I must move from ky to

@sholladay
Copy link
Collaborator

Actually, it should probably be /myapp/api

@alexgleason
Copy link

Man this library is awesome, but this is the one thing arguably the most important to get right. I just changed hundreds of lines of code trying to switch from Axios and I'm stuck on this. I even removed the leading slash from everything (which felt very wrong btw), but there's no workaround for absolute URLs. I tried overriding prefixUrl to '' on just the calls with absolute URLs, and I tried the beforeRequest hook too but it's too late by then. I can either fork or wrap the wrapper. I think what Axios does is what people want.

@sholladay
Copy link
Collaborator

sholladay commented Oct 9, 2024

@alexgleason I have a branch that is intended to solve this problem and make everyone happy. Please give it a try and send us feedback.

See PR #606.

It is not ready to be merged yet, but it works and is fully tested. I just need to find some time to work on the docs and TS types.

Here is how it works:

  • There is now an option for input prefixing (startPath) and an option for URL resolution (baseUrl). They can be used together but generally you will only need one or the other.
  • The startPath option is a prefix for the input, and the input is always joined to it by a single, normalized slash, meaning input slash validation has been removed and the resulting URL is the same whether or not the input has a leading slash.
  • The baseUrl option behaves exactly like the HTML <base> tag would, meaning it is sensitive to leading and trailing slashes and absolute URLs, the same way fetch() and other web APIs are.
  • The joining of startPath and input happens before it is resolved against baseUrl.

Having both options provides quite a bit of flexibility. However, we want to avoid any confusion between these two options, so please let me know what you think of the names and whether there's any ambiguity.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants