-
Notifications
You must be signed in to change notification settings - Fork 41
searchApi: fix "url" axios config override #89
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
Conversation
slint
commented
Jan 17, 2020
- Closes InvenioSearchApi: accidental use of both "baseURL" and "url" config #88.
289e6a1 to
84847fc
Compare
| } | ||
|
|
||
| initAxios(config) { | ||
| initAxios({ url, ...config }) { |
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.
@slint How is this different than the existing implementation?
Before the code was expecting an object e.g {url: '/me'} and this would end up in the non desired behaviour of calling /me/me when it was triggering a request. Now with the change passing the passing the same object I think it would lead to the same outcome no? Unless you pass explicitly the baseURL parameter which in that case it was already possible before...Do I miss 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.
Since now url is a destructured value from the passed object function argument, it will not be inside the ...config object. This means that it won't be passed at all to the axios.create(...) call
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.
@slint But then you will not be able to pass the url parameter at all 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.
Yup. We have to discuss whether we'll directly pass-through the config to axios.create(...), or if we'll introduce an axiosConfig parameter to the ESSearchApi/InvneioSearchApi class constructor.
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.
@ntarocco what do you think? I would be in favor I think of name spacing the axios config like axiosConfig and pass it through to axios.
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.
Are we discussing this? #88
If yes, I should have already replied there :)
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.
@ntarocco sorry I missed that. So just to confirm what you are proposing is just get through all the config passed to the axios instance right? In that case I wouldn't destructure the url parameter from the config passed and just pass through the config as it is. Then, it would be clear for the users (we should update the docs for that too) that this config is the same with what axios is accepting. @slint WDYT?
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.
I would go with an explicit axiosConfig parameter for the InvenioSearchApi constructor:
react-searchkit/src/lib/api/contrib/invenio/InvenioSearchApi.js
Lines 15 to 16 in 375f51d
| export class InvenioSearchApi { | |
| constructor(config) { |
So something like:
export class InvenioSearchApi {
constructor(serializers, axiosConfig, axiosInterceptors) {
// NOTE: Would still validate the axiosConfig, but inside "initAxios"
// this.validateConfig(config);
this.initSerializers(serializers);
this.initAxios(axiosConfig, axiosInterceptors);
}
...
}|
LGTM |