elbywan / wretch

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

How to throw custom errors using the error() handler without trigger the final catch. #61

Closed MarlBurroW closed 4 years ago

MarlBurroW commented 4 years ago

Hi, I would like to throw some custom errors on some specific http response codes (eg: 422 here), but I also need to throw a more generic error for all other status codes. So I tried something like this:

// a very simplified version of my client API layer
wretch("anything")
  .errorType('json')  
  .post(somedata)
  .error(422, error => {throw new ServerValidationError(error)}
  .res(response => /* ... */)
  .catch(error => {
      throw HttpError(error)
   })

But throwing a 422 error reject the promise and trigger the catch callback at the end (that is the usual behavior on a promise based lib), but then, I got a HttpError instead of a ServerValidationError when I .catch() error on top of my api service.

I was inspired by this example in the documentations:

wretch("anything")
  .get()
  .notFound(error => { /* ... */ })
  .unauthorized(error => { /* ... */ })
  .error(418, error => { /* ... */ })
  .res(response => /* ... */)
  .catch(error => { /* uncaught errors */ })

For me, this example means

If response status code is 418, perform the attached callback, but do not fire the catch(). catch() is only for errors that are not handled by the error() or named errors handlers

But it's probably a misunderstanding of this example, so I changed my code to something like this.

wretch("anything")
  .errorType('json')  
  .post(somedata)
  .res(response => /* ... */)
  .catch(error => {
    if (error.response) {
      switch (error.response.status) {
        case 422:
          throw new ServerValidationError(error)
          break
        default:
          throw new HttpError(error)
          break
     }
    } else {
      throw new NetworkError(error)
    }
  })

This works fine but i'm not sure it is the best way to achieve my goal, is there a better way to do this ?

Thanks

elbywan commented 4 years ago

Hi @MarlBurroW,

For me, this example means (...)

catch() is only for errors that are not handled by the error() or named errors handlers

The .catch() function in the example is the vanilla Promise.prototype..catch() function. It is meant to catch any error thrown that was uncaught by other mechanisms.

If response status code is 418, perform the attached callback, but do not fire the catch().

What triggers the catch() is the fact that you are re-throwing the error inside the callback. Not throwing and returning a response-like value works.

This works fine but i'm not sure it is the best way to achieve my goal, is there a better way to do this ?

There are 2 ways of achieving this that come to my mind right now.

1) I think that a better way would be to flag your custom error classes and then instead of the big pattern matching case/switch/if inside just write a two line condition.

Something like (reusing your simplified example):

wretch("anything")
  .errorType('json')  
  .post(somedata)
  .error(422, error => {
    // Just flag the ServerValidationError class with a field.
    // Let's say .custom = true
    throw new ServerValidationError(error)
   }
  .res(response => /* ... */)
  .catch(error => {
    if(error.custom)
      throw error
    throw HttpError(error)
   })

2) And a maybe even better way would be to take advantage of the Promise then/catch chaining, removing the need to flag anything:

wretch("anything")
  .errorType('json')  
  .post(somedata)
  // Instead of throwing, just return the error
  .error(422, error => new ServerValidationError(error)
  .res(response => /* ... */)
  .catch(error => { throw HttpError(error) })
  // Use .then here
  .then(result => {
    if(result instanceof Error)
        throw result
    return result
  })
MarlBurroW commented 4 years ago

Thank you very much for your answer and your explanations !

I will use your last example which sounds good to me (I didn't thought to return the error instance without throwing it).

Thank for your work

th3fallen commented 2 years ago

sorry to reopen this dinosaur, but I'm trying to do something very similar except at a global level I've tried using .catcher but that doesn't appear to get me the win I want. is there any way to modify the error payload in a middleware for instance?

just like elbywan was attempting I want to throw a custom error class down to anything that uses this wretch client.

elbywan commented 2 years ago

Hey @th3fallen 👋

sorry to reopen this dinosaur

No worries!

I'm trying to do something very similar except at a global level I've tried using .catcher but that doesn't appear to get me the win I want is there any way to modify the error payload in a middleware for instance?

Could you please post a code sample of what you are trying to achieve? I'm not sure I understand the use case without a concrete example.

th3fallen commented 2 years ago

long short is i want to be able to do something like...

adapter.ts

import defaultRequestConfig from 'shared/api/adapters/wretch/defaultRequestConfig';
import QueryStringAddon from './QueryStringAddon';

import wretch from 'wretch';
import AbortAddon from 'wretch/addons/abort';
import formData from 'wretch/addons/formData';
import PerformanceAddon from 'wretch/addons/perfs';
import { dedupe } from 'wretch/middlewares/dedupe';
import { throttlingCache } from 'wretch/middlewares/throttlingCache';

export default (baseUrl: string) => wretch(baseUrl, defaultRequestConfig())
  .errorType('json')
  .resolve(res => res.json())
  .addon(QueryStringAddon)
  .addon(formData)
  .addon(AbortAddon())
  .addon(PerformanceAddon())
  .middlewares([
    dedupe({
      skip: (url, opts) => opts.skipDedupe || opts.method !== 'GET',
      key: (url, opts) => `${ opts.method }@${ url }`,
      resolver: response => response.clone(),
    }),
    throttlingCache(),
  ])
.catch(e => {
return mutateError(e) < this would get the error and format some values with accessors using the below custom response
});

errorResponse.ts

export class ErrorResponse extends Error {
  response = null;
  body = null;

  constructor(response, body) {
    super(response.statusText);
    this.name = 'ErrorResponse';
    this.response = response;
    this.body = body;
  }
}

export class ErrorResponseGoSingle extends ErrorResponse {
  constructor(response, body) {
    super(response, body);
    this.name = 'ErrorResponseGoSingle';
    this.code = body?.code;
    this.message = body.message;
  }
}

/**
 * Some endpoints (usually but not always in the monolith) return a JSON body with a single "error" string property
 *
 * Access with: errorResponseOne.body.error
 */
export class ErrorResponseOne extends ErrorResponse {
  constructor(response, body) {
    super(response, body);
    this.name = 'ErrorResponseOne';
    this.message = body.error;
  }
}

/**
 * Some endpoints (particularly Golang, some monolith) return a JSON body with an "errors" property containing an
 * array of { code: "string", message: "string" } shaped objects
 *
 * Access with: errorResponseMany.body.errors
 */
export class ErrorResponseMany extends ErrorResponse {
  constructor(response, body) {
    super(response, body);
    this.name = 'ErrorResponseMany';
    this.message = body.errors.map(err => err.message ? err.message : err.toString()).join('\n');
  }
}

the ultimate goal being the client could do adapter.query(asdfasd).get().catch(e => e would be one of the above error classes)

elbywan commented 2 years ago

long short is i want to be able to do something like...

Thanks for the sample 👍

the ultimate goal being the client could do adapter.query(asdfasd).get().catch(e => e would be one of the above error classes)

How about calling .catch in the resolver ?

export default (baseUrl: string) => wretch(baseUrl, defaultRequestConfig())
  .errorType('json')
  .resolve(res => res.json().catch(error => { throw mutateError(e) })
  //…
th3fallen commented 2 years ago

you are a gentleman and a scholar, no idea how that didn't dawn on me but that works perfectly.