search

buildo / metarpheus-io-ts

Generate domain models and client interpreting metarpheus output
MIT License
7 stars 1 forks source link

#91: generate TaskEither APIs (instead of Promise) (closes #91) #92

Closed giogonzo closed 5 years ago

giogonzo commented 5 years ago

Closes [#2522358](https://buildo.kaiten.io/space/[object Object]/card/2522358)

⚠️ The line above was added during the automatic migration of all github project issues to Kaiten. The old GH issue link is kept here for reference (it is now deprecated, continue follow the original issue on Kaiten).

Closes #91

Test Plan

tested this at runtime on a project locally: https://github.com/buildo/getjenny/compare/test-metarpheus-taskeither

tests performed

A Test Plan is used to show what you tested to make sure your code works fine. You should write here all that you did to test, and provide some results of your testing.

These results can be screenshots, query results, or even just pointers to unit tests that successfully passed.

tests not performed (domain coverage)

At times not everything can be tested, and writing what hasn't been tested is just as important as writing what has been tested.

An example of partial test is a field displaying 4 possible values. If 3 values are tested, with screenshots, and 1 is not, then it should be mentioned here.

giogonzo commented 5 years ago

maybe we expose something that's easier to work with for clients, instead of an untagged union

Yes, my rationale here is: clients are interested in handling

  1. "network" errors
  2. "application-level" errors
  3. but NOT "io-ts response decoding" errors: they would mean either a bug in metarpheus-io-ts generated code OR the client running against an invalid API version (and I believe both these cases should be handled somehow but not at application level?)

1 & 2 are currently conflated into AxiosError, at least until we don't agree upon a standard encoding of failures in metarpheus/fullstack - e.g. a solution would be wait for union types support then encode responses as HTTP 200 with an Either<AppError, Output> body

3 is currently in this PR mainly because I wanted to discuss it, but for the reasons above I would remove it from the returned L (and just throw at runtime), effectively leaving only AxiosError for the moment.

we don't allow clients to decide how to unwrap the API response type anymore

This is completely unrelated, I just noticed we are not using this in projects anymore. But it should be moved to a sperate PR and released together in the same breaking version

gabro commented 5 years ago

3 is currently in this PR mainly because I wanted to discuss it, but for the reasons above I would remove it from the returned L (and just throw at runtime), effectively leaving only AxiosError for the moment.

👍 yeah, I would keep doing as we did before, and throw on failed decoding

This is completely unrelated, I just noticed we are not using this in projects anymore. But it should be moved to a sperate PR and released together in the same breaking version

👍 yep, although conflating this into this PR is not the end of the world