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

Prevent 'undefined' from being appended to API resource URL when using custom Axios instance #45

Merged
merged 1 commit into from
Dec 3, 2017

Conversation

slurmulon
Copy link
Contributor

@slurmulon slurmulon commented Nov 30, 2017

Problem

When using a custom axios instance it's possible for the final resource URL to include an unwanted undefined string literal, like so:

Unwanted result: http://localhost:8080/undefined/v1/user
Desired result: http://localhost:8080/v1/user

This only happens when using a custom axios instance because, in this case, the user typically avoids setting the core baseURL property and instead only sets the property in axios. Ideally users should only set one or the other.

Solution

This proposed change corrects the problem by introducing the normalizedBaseURL getter.

This method explicitly checks for and utilizes the baseURL value defined in axios.

@slurmulon
Copy link
Contributor Author

I'm not sure if this belongs in v2 or master, seems like probably master instead..?

@christianmalek
Copy link
Owner

Hello @slurmulon,

thanks for your PR. I really appreciate it!

Currently master branch equals v1. So you PR'ed properly to v2. :)

I'll investigate the PR later this week and let you know when I'm merging it.

@slurmulon
Copy link
Contributor Author

@christianmalek Cheers, no problem at all. Thank you for all of your awesome work!

@christianmalek christianmalek merged commit b00d549 into christianmalek:v2 Dec 3, 2017
@christianmalek
Copy link
Owner

@slurmulon It's merged and I updated the README.

@slurmulon
Copy link
Contributor Author

@christianmalek Awesome, thanks!

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

Successfully merging this pull request may close these issues.

2 participants