feat(useFetch): update data on success #2711
Conversation
antfu
added
the
direction: approved
|
Maintainers, what it your plan for this issue? I have created a PR that solves it: #2711. Can you either approve / disapprove? |
| @@ -349,7 +349,7 @@ export function useFetch<T>(url: MaybeComputedRef<string>, ...args: any[]): UseF | |||
| const statusCode = ref<number | null>(null) | |||
| const response = shallowRef<Response | null>(null) | |||
| const error = shallowRef<any>(null) | |||
| const data = shallowRef<T | null>(initialData) | |||
| const data = shallowRef<T | null>(initialData || null) | |||
There was a problem hiding this comment.
Be careful, if initialData is falsy, it will be set as null, not expected.
E.g. if I set { initialData: false } as option, then data will be set to null instead of false. Is it possible to use ?? instead of ||?
There was a problem hiding this comment.
Sure - good catch. I will create another PR for this.
|
FYI this is a BREAKING CHANGE and should have been documented as such. It is quite normal for an API to return error information along with an error status. Previously we could retrieve that response in |
|
Also note value of |
|
I honestly don't even see a way (other than terrible hacks) to get the response when an error status is returned. If so, this change was quite unfortunate. |
|
I agree with @aparajita. This is an undocumented breaking change that caused a lot of headaches for us. We downgraded Not sure why this change was even needed... In our use case request can receive validation errors with 422 status code and actual validation errors as payload. We receive the errors list and display to the user. Now with this change, firstly, it is broken, secondly we can no longer access to |
|
Open for PR to add a options to configure this behaviour |
|
@orkhanahmadov I worked around it by attaching the response data to a property on the error object passed to onFetchError. |
@antfu If you are willing to take a look, I will be happy to submit one. |
No @pbrzosko, it isn't. You are only doing that in the It really would be nice if you would go back and fix this mess. You broke error handling for everyone who uses this function. |
|
Sure, let's directly discuss in the PR. Thanks! |
@aparajita Not true. Value of error is fetchError.message || fetchError.name or whatever you set it to be in onFetchError callback. |
@aparajita No hacks needed. You just need to set whatever error you want in onFetchError. The main reason for change was that if you expect to receive apples from an API, you want it to be apples, not oranges if there is an error. So apples (data) should. be in data ref, and error (oranges) should be in the error ref. Event data ref is typed to be T, but apparently sometimes it is something else. I don't think that is right behavior and thus this change. If you do not use typescript, but javascript only and expect anything to be there, maybe you do not have such an issue. |
@aparajita I couldn't make it in the catch block, because then for a single moment data ref would hold the error (again not what it supposed to hold). But I agree, that it also should be set in catch block to the default value. |
@orkhanahmadov And don't you think it is weird, that sometimes you have response type in data (T) and sometimes a list of errors? |
You are ignoring the possibility that the API might return an error status AND a response body to give more information about what the error was. fetchError is useless in such a case. |
@pbrzosko I don't think I do. If the endpoint is returning data alongside a non-successful status code, I would assume I can also access that data easily just the same way I access the status code. Take my validation errors example: yes endpoint does return non-ok code, but it also returns important validation errors that I need to access and use. Unsetting the data on error only complicates the implementation of this. |
|
@pbrzosko now trying to implement import { useFetch } from '@vueuse/core'
useFetch('localhost:8081', {
headers: { foo: 'bar' },
onFetchError(ctx) { // defining `onFetchError` ignores `headers`. they won't get used
return ctx
}
})
useFetch('localhost:8081', {
headers: { foo: 'bar' }, // without `onFetchError`, headers being used
})Am I missing something here? Seems like a bug. Currently with v10 if the endpoint returns a non-successful code with a body, seems like there's no way to access the body while also using headers. I'll open a dedicated issue for this. |
Exactly my point. There has to be a mechanism to retrieve the body when there is an error status. I'm fine with keeping it separate from data, as that actually simplifies the typing, but there should be a clean way to get the response body when response.ok === false. |
|
I've spent the last few hours puzzling over this myself. We have a 422 error being returned, and I need to access body data (validation) Can anyone recommend a good pattern to work around this issue? Or should I simply downgrade? |
@danimalweb We tried to implement |
I do this: interface APIError {
error: number
message: string
}
type ExtendedError = Error & { apiError?: APIError }
useFetch(url, {
onFetchError(ctx: {
data: string | APIError
response: Response | null
error: ExtendedError
}) {
// If the response is a string and is supposed to be JSON,
// try to parse it as JSON. We need to do this because many of our
// endpoints return a success status and no body on success, and
// an error status and JSON error information in the body. If you try to
// use .json() on an empty body, useFetch throws an exception and breaks.
const contentType = ctx.response?.headers.get('content-type') ?? ''
if (contentType.startsWith('application/json')) {
if (typeof ctx.data === 'string') {
try {
// eslint-disable-next-line @typescript-eslint/no-unsafe-assignment -- We're parsing JSON.
ctx.error.apiError = JSON.parse(ctx.data)
} catch {
// Ignore
}
} else {
// Already parsed, save it
ctx.error.apiError = ctx.data
}
}
return ctx
},The |
|
Finally found why Fetch is treating
And yes, it is actually shipping breaking change as bugfix and also ruins the API expectation. |
Description
When people use useFetch, they expect certain type to be returned by the request, ie. an array of options to be provided to the select component. But this data is only present on successfull responses. In case of error, probably a totally different payload is returned. Having this payload set as a data, causes a lot of issues, because it is no longer what client of the useFetch expected.
Additional context
IMHO data in case of error could be provided as a separate state. But in this PR, data is set to initialData provided in options or null, when an error happens.
Payload, when error happened, is provided to the onFetchError callback, so client of the useFetch composable can do whatever is needed to transform provided payload to some meaningful error.
What is the purpose of this pull request?
Before submitting the PR, please make sure you do the following
fixes #123).