contactlab / appy

A functional wrapper around Fetch API
https://contactlab.github.io/appy
Apache License 2.0
63 stars 4 forks source link

Forward `RequestInfoInit` to `ResponseError` #743

Closed valeriobelli closed 11 months ago

valeriobelli commented 11 months ago

Is your feature request related to a problem? Please describe.

While developing a new combinator to log errors generated by appy, I realized that accessing the RequestInfoInit is impossible when handling a ResponseError. At the same time, the same is available in RequestError. Having the information about the Request is highly important when logging the failure to have a complete picture of what happened during the communication with the API.

Describe the solution you'd like

Extending the ResponseError by adding the RequestInfoInit as a property as done with RequestError. However, this will force the library to introduce a breaking change in toResponseError interface.

Describe alternatives you've considered

None.

Additional information/context

StefanoMagrassi commented 11 months ago

@valeriobelli thank you: this is a use case that the library should cover.

Even if the package is not widely used, I'm concerned about introducing a breaking change like this right now.

If you need this feature a.s.a.p. we can add an optional input field to the ResponseError interface in order to be retro-compatible. Then, this field would be set as required in the next major version of the library.

The drawback would be that you will check in your code that the field is defined every time you will access to a ResponseError object.

valeriobelli commented 11 months ago

Ciao @StefanoMagrassi 👋🏽 Thanks for the answer.

It's an excellent trade-off to avoid an immediate breaking change and let Appy's consumers adapt to the new interface.

StefanoMagrassi commented 11 months ago

@valeriobelli I pushed the changes in the feature/743-forward-requestinfoinit-to-responseerror branch: I already tested them with our huge application and everything worked fine without any intervention.

The new optional input field was added either to ResponseError and Resp interfaces because, due to the contravariant nature of ReaderTaskEither, any combinator that acts on the request's input will "override" the original one when you try to read it and the information would be lost.

(Actually, the input is not "overridden", but because of how pipe() is implemented when the combinator is executed the data is temporary. Only when the underlying fetch is ran and the response read you would be sure that all combinators were been executed and the input has been built correctly.)

Please, check it out and tell me if it works for you.

StefanoMagrassi commented 11 months ago

@valeriobelli in order to test the branch locally, I have to pull it on your machine and then run:

$ npm ci
$ npm run build
$ cd ./dist
$ npm pack

The build command will create a "compiled" version of the library in the /dist folder and into this directory you can run the pack command that will create a .tg.zip file.

Then, you can install this tarball in your application:

$ npm i ../path/to/tarball
valeriobelli commented 11 months ago

@StefanoMagrassi It tested against our test battery of some Appy's abstractions, and it worked like a charm.

Having the input even for the Resp is an excellent addition to this library.

Thank you for the prompt effort. 💯