elbywan / wretch

A tiny wrapper built around fetch with an intuitive syntax. :candy:
MIT License
4.83k stars 98 forks source link

Feature: option to disable default errorType() #118

Closed gbcreation closed 2 years ago

gbcreation commented 2 years ago

First of all, thank you for making Wretch. That's a very useful Fetch wrapper.

When an error Response is returned by fetch(), Wretch always consumes it through a call to text() or json() depending on the configuration of errorType(). However, it can be useful some times to get the unconsumed raw response and let the error catcher handle it.

Currently, it is not possible to disable the consuming of the reponse. Maybe a call to errorType() without parameter could be used for that. WDYT?

elbywan commented 2 years ago

Hey @gbcreation,

First of all, thank you for making Wretch. That's a very useful Fetch wrapper.

Thanks! 🙇

However, it can be useful some times to get the unconsumed raw response and let the error catcher handle it. Maybe a call to errorType() without parameter could be used for that. WDYT?

I think this is a very uncommon usecase, and using errorType() without parameter would be breaking since right now it defaults to calling .text() on the Response. So I'm not keen on doing that…

Currently, it is not possible to disable the consuming of the reponse.

…but the good thing is that you can achieve this already using a middleware 😄.

// Define a middleware that will intercept the response.
const errorMiddleware = next => (url, opts) => {
  return next(url, opts).then(response => {
    // Define a "_msg" function that will resolve to an empty string.
    // If you want a custom error message you can put anything inside…
    response._msg = () => Promise.resolve("")
    return response
  })
}

// Configure the middleware and the errorType with the "_msg" function here…
const w = wretch("https://httpstat.us").errorType("_msg").middlewares([errorMiddleware])

w.url("/400").get().res().catch(error =>
  // Access the response using the `.response` property.
  // it will not be consumed since "_msg" did not touch the body.
  // Here we consume it manually by calling ".text()"
  error.response.text()
).then(txt => console.log(txt))
gbcreation commented 2 years ago

Hi @elbywan

Thank you for your suggestion. It works perfectly, but looks a little bit hacky IMO because we have to circumvent the fact that Wretch always processes the error Response. Here, a processing is performed to explicitly tell Wretch to do nothing, while the same result could be achieved if Wretch would indeed do nothing :)

I agree calling errorType() without parameter is not the best option because of the default behavior that calls .text(), but maybe a new 'raw" or "unconsumed" or ... ("better name" ?) parameter that would make Wretch let the error Response unconsumed would be a better approach.

gbcreation commented 2 years ago

When calling .errorType("_msg"), TypeScript complains with Argument of type '"_msg"' is not assignable to parameter of type '"text" | "json"'. Do you know how to tell TypeScript that "_msg" is an acceptable argument?

elbywan commented 2 years ago

Do you know how to tell TypeScript that "_msg" is an acceptable argument?

The bindings are wrong here, I'll need to update them to accept any string as an argument. In the meantime "_msg" as any should work. Thanks for reporting 👍.

Here, a processing is performed to explicitly tell Wretch to do nothing, while the same result could be achieved if Wretch would indeed do nothing :)

Wretch needs to populate the error message actually.

I agree calling errorType() without parameter is not the best option because of the default behavior that calls .text(), but maybe a new 'raw" or "unconsumed" or ... ("better name" ?) parameter that would make Wretch let the error Response unconsumed would be a better approach.

Again I think that the use case is so uncommon that it is not really worth it IMO. And adding a "magic string" argument sounds even more hacky to me 😉 .

gbcreation commented 2 years ago

The bindings are wrong here, I'll need to update them to accept any string as an argument. In the meantime "_msg" as any should work.

Thank you for this temporary solution.

And adding a "magic string" argument sounds even more hacky to me :wink: .

Well, maybe a new .processError(true | false) (default is true) method would then be even less hacky :smile:

elbywan commented 2 years ago

@gbcreation Typescript bindings have been updated and released with v1.7.7 📦 ✨.

Regarding the original feature request, since it is a very uncommon use case and quite easily done using a simple middleware I'm sorry but I'm not up for implementing that in the lib itself (& by extension increasing the bundle size).

gbcreation commented 2 years ago

Thanks for the types update.

Closing this issue as your suggested solution based on a middleware works.