Simplify how baseURL works and support URL paths with colons near the front #95
Conversation
… front The default resolveURL is more complex than one would hope because it tries to use the two-argument URL constructor to combine a path with a base URL, but for some reason back in 259206037 we decided that a leading slash in the given path should be interpreted as relative to the full base URL rather than just the host (ie, to treat it the same as no leading slash), which breaks the algorithm performed by `new URL`. We worked around it by messing around with slashes a bit but it turned out if you passed `/foo:bar` as your path, it would end up treating that as equivalent to `foo:bar`, which it interprets as an URL with scheme `foo`! We go for an approach that is in some ways much simpler. After this change, the URL that is chosen is exactly the same as the URL you'd get to by clicking a link with that value on a web page with the given base URL. When the base URL doesn't contain a path, this doesn't change the behavior. But it does change the behavior if there is a path in a few ways: - If the path passed to a method such as `this.get()` starts with a slash, then it is resolved relative to the *host* of the base URL, not to the full base URL. That is, if `baseURL` is https://foo.com/a/b/c/, `this.get('d')` resolves to https://foo.com/a/b/c/d, but `this.get('/d')` resolves to https://foo.com/d - If the base URL has a path element and does not end in a slash, then the given path replaces the last element of the path. That is, if `baseURL` is https://foo.com/a/b/c, `this.get('d')` resolves to https://foo.com/a/b/d We think making this into a simple rule that matches browser behavior (and fixes use with paths that have a colon in the first segment) is a reasonable major-version change. Fixes #23.
| nock(apiUrl).get('/api/foo').reply(200, {}); | ||
|
|
||
| await dataSource.getFoo(); | ||
| }); | ||
|
|
||
| it('can use a whole new URL, overriding baseURL', async () => { |
There was a problem hiding this comment.
I believe this worked before and just wasn't tested.
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. |
|
I have mixed feelings about this. On the one hand you can work around #23 with a The problem before is that |
There was a problem hiding this comment.
Well explained PR, very helpful changelog entry. I agree with the decision to make this breaking change and appreciate the simplicity and removal of magic this introduces, especially since you've provided a copy/pastable migration path to preserve old behavior.
The default resolveURL is more complex than one would hope because it tries to use the two-argument URL constructor to combine a path with a base URL, but for some reason back in 259206037 we decided that a leading slash in the given path should be interpreted as relative to the full base URL rather than just the host (ie, to treat it the same as no leading slash), which breaks the algorithm performed by
new URL. We worked around it by messing around with slashes a bit but it turned out if you passed/foo:baras your path, it would end up treating that as equivalent tofoo:bar, which it interprets as an URL with schemefoo!We go for an approach that is in some ways much simpler. After this change, the URL that is chosen is exactly the same as the URL you'd get to by clicking a link with that value on a web page with the given base URL.
When the base URL doesn't contain a path, this doesn't change the behavior. But it does change the behavior if there is a path in a few ways:
this.get()starts with a slash, then it is resolved relative to the host of the base URL, not to the full base URL. That is, ifbaseURLis https://foo.com/a/b/c/,this.get('d')resolves to https://foo.com/a/b/c/d, butthis.get('/d')resolves to https://foo.com/dbaseURLis https://foo.com/a/b/c,this.get('d')resolves to https://foo.com/a/b/dWe think making this into a simple rule that matches browser behavior (and fixes use with paths that have a colon in the first segment) is a reasonable major-version change.
Fixes #23.