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.

Sign up for GitHub

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

Jump to bottom

Fixing combineURLs to support an empty relativeURL #581

Merged
merged 2 commits into from
Dec 8, 2016
Merged

Fixing combineURLs to support an empty relativeURL #581

merged 2 commits into from
Dec 8, 2016

Conversation

loklaan
Copy link
Contributor

@loklaan loklaan commented Dec 7, 2016

Ending slashes were always being appended to the baseURL, regardless of if the relative URL had length.

By making sure that we're supporting empty relative urls, and not appending extraneous slashes to the base url, support for absolute API endpoints becomes possible.

Added a single test spec. 📈

Closes #574

@loklaan
Fixing combineURLs to support an empty relativeURL
2bd48d4 
When combining the base and relative URLs, we should forego force
appending a slash to the base when the relative URL is empty.
This leads to a semantic url.
@loklaan loklaan mentioned this pull request Dec 7, 2016
Closed
nickuraltsev
nickuraltsev reviewed Dec 7, 2016
View reviewed changes
lib/helpers/combineURLs.js
@@ -8,5 +8,8 @@
* @returns {string} The combined URL
*/
module.exports = function combineURLs(baseURL, relativeURL) {
return baseURL.replace(/\/+$/, '') + '/' + relativeURL.replace(/^\/+/, '');
var end = relativeURL.replace(/^\/+/, '');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If relativeURL is '/', it will not be added, right? For example:

combineURLs('http://test.com/foo', '/'); // => 'http://test.com/foo'

I would suggest to append relativeURL unless it's falsy ('', undefined, null). Thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought about this, and covered the case by using the full baseURL (it could include an ending slash).

combineURLs('http://test.com/foo/', ''); // => 'http://test.com/foo/'

But yes I think it may be confusing without the support for your suggestion.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool, could you please update the PR?

nickuraltsev
nickuraltsev requested changes Dec 7, 2016
View reviewed changes
Copy link
Member

@nickuraltsev nickuraltsev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the PR! Please see my comment.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.3%) to 93.924% when pulling 2bd48d4 on loklaan:support-base-url-only into cfe33d4 on mzabriskie:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.3%) to 93.909% when pulling 8816bf6 on loklaan:support-base-url-only into cfe33d4 on mzabriskie:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.3%) to 93.909% when pulling 75965ff on loklaan:support-base-url-only into cfe33d4 on mzabriskie:master.

@loklaan
Copy link
Contributor Author

loklaan commented Dec 8, 2016

Hey @nickuraltsev, this should be good to go now! Squashing the commits on merge would be good.

@nickuraltsev nickuraltsev merged commit fe7d09b into axios:master Dec 8, 2016
@nickuraltsev
Copy link
Member

@loklaan Thanks a lot for the contribution!

@loklaan
Copy link
Contributor Author

loklaan commented Dec 8, 2016

Thanks for the help! Hope to contribute more via #520. :)

@TylerBrock
Copy link

Can you do a release for this one? It's a very common use case.

@renovate renovate bot mentioned this pull request Mar 22, 2018
Merged
jimthedev referenced this pull request in commitizen/cz-cli May 24, 2018
@renovate @jimthedev
chore(deps): update dependency axios to v0.18.0 (#488)
d92b728 
This Pull Request updates dependency [axios](https://github.com/axios/axios) from `v0.15.2` to `v0.18.0`



<details>
<summary>Release Notes</summary>

### [`v0.18.0`](https://github.com/axios/axios/blob/master/CHANGELOG.md#&#8203;0180-Feb-19-2018)
[Compare Source](axios/axios@v0.17.1...v0.18.0)
- Adding support for UNIX Sockets when running with Node.js ([#&#8203;1070](`https://github.com/axios/axios/pull/1070`))
- Fixing typings ([#&#8203;1177](`https://github.com/axios/axios/pull/1177`)):
    - AxiosRequestConfig.proxy: allows type false
    - AxiosProxyConfig: added auth field
- Adding function signature in AxiosInstance interface so AxiosInstance can be invoked ([#&#8203;1192](`https://github.com/axios/axios/pull/1192`), [#&#8203;1254](`https://github.com/axios/axios/pull/1254`))
- Allowing maxContentLength to pass through to redirected calls as maxBodyLength in follow-redirects config ([#&#8203;1287](`https://github.com/axios/axios/pull/1287`))
- Fixing configuration when using an instance - method can now be set ([#&#8203;1342](`https://github.com/axios/axios/pull/1342`))

---

### [`v0.17.1`](https://github.com/axios/axios/blob/master/CHANGELOG.md#&#8203;0171-Nov-11-2017)
[Compare Source](axios/axios@v0.17.0...v0.17.1)
- Fixing issue with web workers ([#&#8203;1160](`https://github.com/axios/axios/pull/1160`))
- Allowing overriding transport ([#&#8203;1080](`https://github.com/axios/axios/pull/1080`))
- Updating TypeScript typings ([#&#8203;1165](`https://github.com/axios/axios/pull/1165`), [#&#8203;1125](`https://github.com/axios/axios/pull/1125`), [#&#8203;1131](`https://github.com/axios/axios/pull/1131`))

---

### [`v0.17.0`](https://github.com/axios/axios/blob/master/CHANGELOG.md#&#8203;0170-Oct-21-2017)
[Compare Source](axios/axios@v0.16.2...v0.17.0)
- **BREAKING** Fixing issue with `baseURL` and interceptors ([#&#8203;950](`https://github.com/axios/axios/pull/950`))
- **BREAKING** Improving handing of duplicate headers ([#&#8203;874](`https://github.com/axios/axios/pull/874`))
- Adding support for disabling proxies ([#&#8203;691](`https://github.com/axios/axios/pull/691`))
- Updating TypeScript typings with generic type parameters ([#&#8203;1061](`https://github.com/axios/axios/pull/1061`))

---

### [`v0.16.2`](https://github.com/axios/axios/blob/master/CHANGELOG.md#&#8203;0162-Jun-3-2017)
[Compare Source](axios/axios@v0.16.1...v0.16.2)
- Fixing issue with including `buffer` in bundle ([#&#8203;887](`https://github.com/axios/axios/pull/887`))
- Including underlying request in errors ([#&#8203;830](`https://github.com/axios/axios/pull/830`))
- Convert `method` to lowercase ([#&#8203;930](`https://github.com/axios/axios/pull/930`))

---

### [`v0.16.1`](https://github.com/axios/axios/blob/master/CHANGELOG.md#&#8203;0161-Apr-8-2017)
[Compare Source](axios/axios@v0.16.0...v0.16.1)
- Improving HTTP adapter to return last request in case of redirects ([#&#8203;828](`https://github.com/axios/axios/pull/828`))
- Updating `follow-redirects` dependency ([#&#8203;829](`https://github.com/axios/axios/pull/829`))
- Adding support for passing `Buffer` in node ([#&#8203;773](`https://github.com/axios/axios/pull/773`))

---

### [`v0.16.0`](https://github.com/axios/axios/blob/master/CHANGELOG.md#&#8203;0160-Mar-31-2017)
[Compare Source](axios/axios@v0.15.3...v0.16.0)
- **BREAKING** Removing `Promise` from axios typings in favor of built-in type declarations ([#&#8203;480](`https://github.com/axios/axios/issues/480`))
- Adding `options` shortcut method ([#&#8203;461](`https://github.com/axios/axios/pull/461`))
- Fixing issue with using `responseType: 'json'` in browsers incompatible with XHR Level 2 ([#&#8203;654](`https://github.com/axios/axios/pull/654`))
- Improving React Native detection ([#&#8203;731](`https://github.com/axios/axios/pull/731`))
- Fixing `combineURLs` to support empty `relativeURL` ([#&#8203;581](`https://github.com/axios/axios/pull/581`))
- Removing `PROTECTION_PREFIX` support ([#&#8203;561](`https://github.com/axios/axios/pull/561`))

---

### [`v0.15.3`](https://github.com/axios/axios/blob/master/CHANGELOG.md#&#8203;0153-Nov-27-2016)
[Compare Source](axios/axios@v0.15.2...v0.15.3)
- Fixing issue with custom instances and global defaults ([#&#8203;443](`https://github.com/axios/axios/issues/443`))
- Renaming `axios.d.ts` to `index.d.ts` ([#&#8203;519](`https://github.com/axios/axios/issues/519`))
- Adding `get`, `head`, and `delete` to `defaults.headers` ([#&#8203;509](`https://github.com/axios/axios/issues/509`))
- Fixing issue with `btoa` and IE ([#&#8203;507](`https://github.com/axios/axios/issues/507`))
- Adding support for proxy authentication ([#&#8203;483](`https://github.com/axios/axios/pull/483`))
- Improving HTTP adapter to use `http` protocol by default ([#&#8203;493](`https://github.com/axios/axios/pull/493`))
- Fixing proxy issues ([#&#8203;491](`https://github.com/axios/axios/pull/491`))

---

</details>




---

This PR has been generated by [Renovate Bot](https://renovatebot.com).
@axios axios locked and limited conversation to collaborators May 3, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@nickuraltsev nickuraltsev nickuraltsev approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

API endpoints with file extensions
4 participants
@loklaan @coveralls @nickuraltsev @TylerBrock