Got: Document the behavior of the `baseUrl` option

[marswong]

in common senarios, we used to deploy our api with a url {host}/api, so we may use got like this:

const client = got.extend({
  baseUrl: 'http://xxx.com/api/v1',
});

client.get('/stuffs')

somehow, cuz the baseUrl just support a WHATWG URL currently, we have to add a /api/v1 prefix for each url and it really sucks for tons of long urls.

[sindresorhus]

That is a bug. I incorrectly assumed the base parameter to new URL() accepted any URL, but seems it just silently throws away the path part...

(new URL('/foo', 'https://sindresorhus/unicorn')).toString();
//=> 'https://sindresorhus/foo'

We should also update the baseUrl docs to be explicit about supporting an URL even with a path.

[sindresorhus]

In my defence, the MDN docs are not clear at all about this:

base - A USVString representing the base URL to use in case url is a relative URL. If not specified, it defaults to ''. - https://developer.mozilla.org/en-US/docs/Web/API/URL/URL

[marswong]

This is a bug.

em, cuz the url module follows WHATWG URL Standard it may not be a bug😂

we just need to support the path part

[sindresorhus]

@marswong I think you misunderstood. It's indeed a bug in Got. We need to handle the path part, yes.

[szmarczak]

@sindresorhus

This example is correct (because baseUrl is missing / in the end).

(new URL('/foo', 'https://sindresorhus/unicorn')).toString();
//=> 'https://sindresorhus/foo'

However this is a common mistake (/ in the beginning points to http://sindresorhus/):

(new URL('/foo', 'https://sindresorhus/unicorn/')).toString();
//=> It returns 'https://sindresorhus/foo' but we expected 'https://sindresorhus/unicorn/foo'

There are three ways to fix this:

1. Don't use / in the beginning:

(new URL('foo', 'https://sindresorhus/unicorn/')).toString();
//=> 'https://sindresorhus/unicorn/foo'

1. Use ./ instead /:

(new URL('./foo', 'https://sindresorhus/unicorn/')).toString();
//=> 'https://sindresorhus/unicorn/foo'

1. As you suggested, we could handle it on our own, but I see no sense. That's how URL works.

but seems it just silently throws away the path part...

<a> tags work the same way: http://jsfiddle.net/gp8ba51w/2/

I'd just update the docs about that behaviour :)

[sindresorhus]

@szmarczak Ah, I totally missed that. I think the current behavior is ok, but we should make it much clearer in the docs.

[marswong]

@szmarczak sorry, have u tried with got? i just removed the / of url, it still sucks :(

[szmarczak]

@marswong Got 9:

(async () => {
    const {body} = await got('get', {baseUrl: 'http://nghttp2.org/httpbin/'});
    console.log(body);

    console.log('---------------');

    const instance = got.extend({baseUrl: 'http://nghttp2.org/httpbin/'});
    const body2 = (await instance('get')).body;
    console.log(body2);
})();

Output:

{
  "args": {},
  "headers": {
    "Accept-Encoding": "gzip, deflate",
    "Connection": "close",
    "Host": "nghttp2.org",
    "User-Agent": "got/9.0.0 (https://github.com/sindresorhus/got)",
    "Via": "1.1 nghttpx"
  },
  "origin": "5.184.0.25",
  "url": "http://nghttp2.org/httpbin/get"
}

---------------
{
  "args": {},
  "headers": {
    "Accept-Encoding": "gzip, deflate",
    "Connection": "close",
    "Host": "nghttp2.org",
    "User-Agent": "got/9.0.0 (https://github.com/sindresorhus/got)",
    "Via": "1.1 nghttpx"
  },
  "origin": "5.184.0.25",
  "url": "http://nghttp2.org/httpbin/get"
}

It works as expected.

[marswong]

@szmarczak i try your sample, it works.

just the baseUrl must be ended with /, or it would throw when meeting dynamic urls🙈, see:

(async () => {
  const instance = got.extend({ baseUrl: 'https://api.douban.com/v2' });
  const id = 1220562;
  const body = (await instance.get(`book/${id}`)).body;
  console.log(body);
})();

so it should be noted in the doc, thanks :)

[beac0n]

In my opinion, this should be

1. documented
2. highlighted in the docs, because the behaviour is quite unintuitive

So the things @szmarczak and @marswong mentioned should be in the docs:
do NOT use / at the beginning
DO use a / at the end

[sindresorhus]

@szmarczak Is there a technical reason the baseUrl has to end in an / other than because that's how new URL() works? If not, I think we should not normalize and add a / to the baseUrl if it doesn't exist. The less unexpected behavior we have to document, the better.

[szmarczak]

Is there a technical reason the baseUrl has to end in an / other than because that's how new URL() works? If not, I think we should not normalize and add a / to the baseUrl if it doesn't exist. The less unexpected behavior we have to document, the better.

It's an expected behaviour, but it may be confusing. I'd leave that as it is.

If you prefer the second way, we could throw an error if baseUrl doesn't end with / :)

[sindresorhus]

It's an expected behaviour, but it may be confusing. I'd leave that as it is.

Why is it an expected behavior? It's not like <a> works. You can have https://sindresorhus/foo (without trailing /) and still have relative links.

[szmarczak]

Why is it an expected behavior?

Why it isn't?

You can have https://sindresorhus/foo (without trailing /) and still have relative links.

This is the current behavior (which is fine). So, the absolute URL is https://sindresorhus/. Visiting bar won't lead to https://sindresorhus/foo/bar but https://sindresorhus/bar.

WHATWG URL says:

A path-absolute-URL string must be U+002F (/) followed by a path-relative-URL string.

So what @marswong wanted is a path-absolute-URL. We could strictly check it and throw if it isn't an absolute URL, but I don't think it's a good idea.

Anyway, like @beac0n has mentioned, I'd just update the docs.

[sindresorhus]

Why it isn't?

I commented that above. Because it's not like how <a> works, and it seems like the expected behavior is split, so why not handle both.

This is the current behavior (which is fine).

I'm confused. We have already established it is not the current behavior. (https://github.com/sindresorhus/got/issues/562#issuecomment-412812259)

So, the absolute URL is https://sindresorhus/. Visiting bar won't lead to https://sindresorhus/foo/bar but https://sindresorhus/bar.

This example is invalid. I can't really reply until I know what you were trying to argue.

Anyway, like @beac0n has mentioned, I'd just update the docs.

Why not just append the / then? I don't see any downside in doing so, and you haven't provided any arguments why we shouldn't do it either.

[szmarczak]

This example is invalid. I can't really reply until I know what you were trying to argue.

Let's skip what I said then, it doesn't really matter because as you've pointed out, we were talking about different things, and I agree.

Why not just append the / then?

That behavior would be confusing (at least for me with the behaviour of the URL class). I'd prefer throwing an error if baseUrl doesn't end with a backslash. It should be strictly defined.

[beac0n]

I'd prefer throwing an error if baseUrl doesn't end with a backslash.

Why? The workflow for the user of the package would then be:
user uses baseUrl without slash => error gets thrown => user changes baseUrl to end with slash

Why not just add a slash at the end of baseUrl if none is present? The outcome would be the same for the user.

We should at most log a warning or something like that.

However, I would prefer to document this thoroughly and the keep the code unchanged, as it would give the user the freedom to use the package as he/she sees fit.
Maybe someone wants this behaviour.

[szmarczak]

Why not just add a slash at the end of baseUrl if none is present? The outcome would be the same for the user.

Please see my previous answer:

That behavior would be confusing (at least for me with the behaviour of the URL class).

Maybe someone wants this behaviour.

I can ask the opposite, right? :)

I don't say no, but just in case to be safe. Similar thing was about the url argument (users thought it could be provided as an option too). So since that we throw now an useful error.

[rarkins]

In my opinion:

• baseUrl should definitely be normalized so that there is no need to end it with a /
• I don't like the URL behaviour and think it's unfortunate if got needs to follow it out of convention

For example, a baseUrl for GitHub Enterprise will normally be like https://github.mycompay.com/api/v3 and meanwhile GitHub always document their APIs with a leading slash:

In my (unmerged) PR to gh-got I supported leading/trailing slashes as follows:

const url = /^https?/.test(path) ? path : opts.endpoint.replace(/\/$/, '') + '/' + path.replace(/^\//, '');

[szmarczak]

Ok, I think we've received enough feedback to make a decision :)

baseUrl should definitely be normalized so that there is no need to end it with a /

So let's do it :+1: We need to document it clearly.

For example, a baseUrl for GitHub Enterprise will normally be like https://github.mycompay.com/api/v3 and meanwhile GitHub always document their APIs with a leading slash

I think it's OK then.