-
-
Notifications
You must be signed in to change notification settings - Fork 6.4k
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
fix(typescript-axios): use baseURL of (custom) axios instance #17296
Conversation
@@ -136,7 +136,7 @@ export const toPathString = function (url: URL) { | |||
*/ | |||
export const createRequestFunction = function (axiosArgs: RequestArgs, globalAxios: AxiosInstance, BASE_PATH: string, configuration?: Configuration) { | |||
return <T = unknown, R = AxiosResponse<T>>(axios: AxiosInstance = globalAxios, basePath: string = BASE_PATH) => { | |||
const axiosRequestArgs = {...axiosArgs.options, url: (configuration?.basePath || axios.defaults.baseURL || basePath) + axiosArgs.url}; | |||
const axiosRequestArgs = {...axiosArgs.options, url: (axios.defaults.baseURL ? '' : configuration?.basePath ?? basePath) + axiosArgs.url}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the axios.defaults.baseURL is non-empty, the resulting url would be an empty string, or am I missing something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the axios.defaults.baseURL is non-empty, the resulting url would be an empty string, or am I missing something?
it should be empty, if you have an axios instance and already set baseURL fot it.
see #16125 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The question we should consider is: if you are using nginx for production forwarding, do you need to concatenate the original domain for it?
If nginx forwarding is not considered, then it should actually be adjusted to? : configuration? .basePath(not null) > operationBasePath > basePath > axios.defaults.baseURL > BASE_PATH
Should you define your own configuration? .basePath(not null)
> operationBasePath
> basePath
The three paths to http://localhost:8080/
, right
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for fixing this bug @mymx2 👍. I think there are way too many variables / configuration here tbh. It's way too complex for no reason. See #16125 (comment)
this pr is fix about use custom axios instance.
If you already have an axios instance, the priority is highest, followed by basePath in the configuration which should allow empty strings.
for final url prefix it would be:
fixcode:
@TiFu (2017/07) @taxpon (2017/07) @sebastianhaas (2017/07) @kenisteward (2017/07) @Vrolijkx (2017/09) @macjohnny (2018/01) @topce (2018/10) @akehir (2019/07) @petejohansonxo (2019/11) @amakhrov (2020/02) @davidgamero (2022/03) @mkusaka (2022/04)