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

No codec to decode status 400 errors using superagent-wrapper+express-wrapper #165

Open
jonball4 opened this issue Jul 13, 2022 · 4 comments

Comments

@jonball4
Copy link
Contributor

jonball4 commented Jul 13, 2022

When using superagent-wrapper in conjunction with a service built on express-wrapper, a 400 error sent by the express-wrapper leads to superagent-wrapper failing to decode (using decodeExpecting) if there is no codec defined for a 400 error response.

It seems like we need a default error codec for the 400 status, because if the request is intercepted and rejected before the defined handler can get ahold of it, then the dev has no referential definition or control of the express-wrapper defined error message coming from request decoding errors which cause 400s.

If the handler returns a 400 under certain circumstances, then that is under the dev's control and can be defined in the HttpRoute definition, but it could potentially conflict with the error response type returned by the express-wrapper, so a default error codec is also not the best solution. I don't have a better suggestion right now, but will think about it...

@jonball4 jonball4 changed the title No codec to decode status 400 errors in express-wrapper No codec to decode status 400 errors using superagent-wrapper+express-wrapper Jul 13, 2022
@bitgopatmcl
Copy link
Contributor

bitgopatmcl commented Jul 18, 2022

Would having decodeExpecting not throw, but instead return something like { status: 400, body: unknown, ... } work for your use case? The unknown body could then be passed through another codec, avoiding the need for express-wrapper superagent-wrapper to bake in assumptions about the body with a default error codec.

@jonball4
Copy link
Contributor Author

jonball4 commented Jul 28, 2022

Would having decodeExpecting not throw, but instead return something like { status: 400, body: unknown, ... } work for your use case? The unknown body could then be passed through another codec, avoiding the need for express-wrapper superagent-wrapper to bake in assumptions about the body with a default error codec.

What would the Expecting part of the name represent in the logic at that point? I think decodeExpecting is useful because it can only return 1 type. Otherwise it seems like it would basically be the same as decode?

What if you define an error codec that is adhered to when sending out an error from the express-wrapper's request decoding and just export that codec? An HttpRoute could by default use that error codec for the 400 status, leaving it up to the caller if they want to redefine the 400 response type by taking a union the existing codec with another, to return 400's that fit their specific use case? Superagent-wrapper would just use the 400 codec defined in the httproute as it does now.

As of right now, basically every route has to look like this:

export const someRoute = httpRoute({
  path: ...,
  method: 'GET',
  request: httpRequest(..)
  response: {
    400: t.partial({}),
  },
});

edit: on second thought you may not want to couple io-ts-http with express-wrapper, so perhaps there is an argument for an express-wrapper specific httpRoute factory method.

@bitgopatmcl
Copy link
Contributor

on second thought you may not want to couple io-ts-http with express-wrapper

Agreed. One missing feature for API specs that could be nice is a way to specify a top-level set of responses that would apply to all routes so then they don't need to be tacked on to every route definition. Then, for example, a service could specify a general 500 error type in one spot. Then, with this, io-ts-http itself could specify a basic decode error type, and express wrapper can just import and use that. Do you think that would work?

@jonball4
Copy link
Contributor Author

nailed it @bitgopatmcl

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

No branches or pull requests

3 participants