-
-
Notifications
You must be signed in to change notification settings - Fork 2.9k
feat(useAxios): Add types and callback to handle AxiosError #4653
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
base: main
Are you sure you want to change the base?
feat(useAxios): Add types and callback to handle AxiosError #4653
Conversation
…AxiosError and add additional computed property to get the AxiosError response data
| isCanceled: Ref<boolean> | ||
| } | ||
| export interface StrictUseAxiosReturn<T, R, D, O extends UseAxiosOptions = UseAxiosOptions<T>> extends UseAxiosReturn<T, R, D, O> { | ||
| export interface StrictUseAxiosReturn<T, R, D, AxiosErrorResponseData = unknown, O extends UseAxiosOptions<T, AxiosErrorResponseData> = UseAxiosOptions<T, AxiosErrorResponseData>> extends UseAxiosReturn<T, R, D, AxiosErrorResponseData, O> { |
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.
this type is breaking
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.
it's unavoidable, do you have any suggestions to make it non breaking?
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.
@OrbisK hello, some time has passed, what is the status of the review? would it be possible to merge it as a breaking change after adding tests? I'm relying a lot on useAxios and this feature is really needed and i also think that many other people need it
|
we should be able to simplify this a whole bunch. i don't think you need to introduce a new type parameter at all. an axios error is basically typed to the request and response (lets call them although im not entirely sure about this change anyway. we already expose the error, we're just doing this so we can filter down to axios errors i suppose? |
yes, but I have to make the check on every request in my codebase.
yes and to be sure that whenever an error is thrown it is related to the response and not to the fact that axios caught an error that is coming from somewhere else E.G the developer uses an interceptor and and a non axios error is thrown there. |
|
I think you misunderstood a little. I was telling you two things:
|
ok I think I understood, I will update the PR soon |
Updated with your suggestions if I understood correctly. so basically now the final data received from a request could be something like |
Before submitting the PR, please make sure you do the following
fixes #123).Description
This PR introduces a generic type system for the AxiosError, in detail:
axiosErrorref that will be populated only when theErroris an instance ofAxiosError, furthermore when the error is not AxiosError, theerrorref will still be populated, and I also added a callback (onAxiosError) that will be executed only when we have anAxiosError, if that is the case, also theonErrorcallback will be executed.AxiosError, basically, according to me, the developer is mainly looking for thedataproperty of the error response, and, also for consistency with thedataproperty of a successfulAxiosResponse, I added a computed propertyaxiosErrorResponseDatathat returns exactly the data of the error response.Example usage
Additional context
If the feature is accepted, I will add tests and update the docs. There is no breaking change in the composable, but only in the types, as it seems unavoidable.
Let me know if the naming is fine, I think they are pretty descriptive, but I'm looking forward to suggestions, if any.