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

useFetch Have access to the response body when the status is not ok #553

Closed
isgj opened this issue Jun 2, 2021 · 15 comments · Fixed by #572
Closed

useFetch Have access to the response body when the status is not ok #553

isgj opened this issue Jun 2, 2021 · 15 comments · Fixed by #572

Comments

@isgj
Copy link
Contributor

isgj commented Jun 2, 2021

At the moment when the status is not ok an error is thrown while fetching. In this case data will be null and the error will be populated. It will be nice to have the body of the response set to data. Even if the status code is not in 200 ...(ok) the response still has a body.

I think just throwing after setting the data will be ok

// see: https://www.tjvantoll.com/2015/09/13/fetch-and-errors/
if (!fetchResponse.ok)
    throw new Error(fetchResponse.statusText)

if (options.afterFetch)      
    ({ data: responseData } = await options.afterFetch({ data: responseData, response: fetchResponse }))

data.value = responseData as any
@isgj
Copy link
Contributor Author

isgj commented Jun 3, 2021

In response to
#549 (comment)
#549 (comment)
(@userquin it will be better to talk here regarding this issue)

One last thing, if the server is down or the client is offline, the response will be null or undefined so we need handle this case, maybe using useOffline before executing the request...

No need at all for this as the then callback won't be executed, will go directly to the catch

For the other concerns, now the body is parsed even when the status is not ok, it's just not set to data

response.value = fetchResponse;
statusCode.value = fetchResponse.status;

let responseData = await fetchResponse[config.type](); //See this line, it's already parsing the body

 // see: https://www.tjvantoll.com/2015/09/13/fetch-and-errors/
if (!fetchResponse.ok)
    throw new Error(fetchResponse.statusText);

if (options.afterFetch)
    ({ data: responseData } = await options.afterFetch({ data: responseData, response: fetchResponse }));
    
 data.value = responseData;

That's why I think, at least for the moment, to throw the error just right after data.value = responseData.
I'm not in favour of introducing other variables, useFetch already has a lot

@userquin
Copy link
Contributor

userquin commented Jun 3, 2021

I'll try to add only one method for Advanced usage, that is, the user must handle the response completely, something like afterFetch with this signature:

export interface CustomResponseFetchContext<T = any> {

  response: Response

  data: T | null

  error: boolean

  errorMessage?: string
}
export interface UseFetchOptions {
  ...
  responseHandler?: (ctx: CustomResponseFetchContext) => Promise<CustomResponseFetchContext>
}

This will be optional on the configuration, and this will be used for response extraction if present on the configuration and will be used instead the current logic, that will remains as is.
This will alow to also handle the error.

@userquin
Copy link
Contributor

userquin commented Jun 3, 2021

The logic to be modified is minimal inside execute function, one if and propagate the error based on error in context.

@userquin
Copy link
Contributor

userquin commented Jun 3, 2021

@isgj how about this?

@userquin
Copy link
Contributor

userquin commented Jun 3, 2021

see a9adad1

@isgj
Copy link
Contributor Author

isgj commented Jun 3, 2021

Something similar might work.
With the proposed implementation afterFetch is not called if responseHandler is provided

@userquin
Copy link
Contributor

userquin commented Jun 3, 2021

Ok I'll add the callback call

@userquin
Copy link
Contributor

userquin commented Jun 3, 2021

Something similar might work.
With the proposed implementation afterFetch is not called if responseHandler is provided

This makes no sense since you are handling the entire response, the after callback is the response handler

@userquin
Copy link
Contributor

userquin commented Jun 3, 2021

I'll mention on docs

@userquin
Copy link
Contributor

userquin commented Jun 3, 2021

@isgj can you take a look at https://github.com/vueuse/vueuse/blob/8d1f05fa232e8580186872c468b4b04b1d16cfb4/packages/core/useFetch/index.md

go below events entry, I added 3 entries, for formEncoded, formData and Advanced usage

@userquin
Copy link
Contributor

userquin commented Jun 3, 2021

@isgj also added warning on Intercepting a request entry using responseHandler.

@userquin
Copy link
Contributor

userquin commented Jun 3, 2021

@isgj second round for docs b5986bc

@schelmo
Copy link
Contributor

schelmo commented Jun 9, 2021

That's why I think, at least for the moment, to throw the error just right after data.value = responseData.
I'm not in favour of introducing other variables, useFetch already has a lot

what about we solve the problem like this:
The response data gets only consumed if the response is ok.

@@ -324,12 +324,12 @@ export function useFetch<T>(url: MaybeRef<string>, ...args: any[]): UseFetchRetu
           response.value = fetchResponse
           statusCode.value = fetchResponse.status

-          let responseData = await fetchResponse[config.type]()
-
           // see: https://www.tjvantoll.com/2015/09/13/fetch-and-errors/
           if (!fetchResponse.ok)
             throw new Error(fetchResponse.statusText)

+          let responseData = await fetchResponse[config.type]()
+
           if (options.afterFetch)
             ({ data: responseData } = await options.afterFetch({ data: responseData, response: fetchResponse }))

then the user can work with the response, since it wasnt consumed

const {onFetchError, data, response} = useFetch('/update').json().put({/*the data*/})
onFetchError(async (error) => {
  // response isnt consumed here, so i can do what i want with it
  // e.g. check validation errors
  if (response.value?.status == 400 || response.value?.status == 422) {
    // get the validation errors
   const validationErrors = await response.value.json()
   console.log(validationErrors)
  }
  else {
    // handle other server errors
  }
})

@isgj
Copy link
Contributor Author

isgj commented Jun 9, 2021

what about we solve the problem like this:
The response data gets only consumed if the response is ok.

Kinda better, at least one can have access to the data (response body).
But you can end with isFetching = false and the data still being parse, so you have to add other checks all over the place to avoid null exceptions. IMHO it won't be a good developer experience.

@isgj
Copy link
Contributor Author

isgj commented Jun 10, 2021

I don't know how I missed it, but I'm closing as duplicate of #526

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