contactlab / appy

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

Question: Why was the error type changed? #598

Closed tmueller closed 2 years ago

tmueller commented 2 years ago

Hi all at contactlab,

thanks for creating such a nice fp-ts wrapper for fetch. One question though: Is there a reason for moving away from the earlier error type export type RequestError = NetworkError | BadUrl | BadResponse;?

Regards, Thomas

StefanoMagrassi commented 2 years ago

Hi @tmueller, thank you.

We decided to drop the previous errors implementation because it was too opinionated.

It all started with the BadUrl type: a 404 response does not always mean that you send a request to a wrong url; maybe the resource you're trying to fetch does not exist, but when you pattern-match the error you're forced to treat it like it.

So, we need a more generic error type and let the consumer handle it properly, by his own needs.

We thought that could be useful anyway to have a base separation between errors generated during the "request" and the "response" phases, so we introduced the RequestError | ResponseError type.

Furthermore, being Err more generic you can always implement a function to transform it in something more specific:

import * as TE from 'fp-ts/TaskEither';
import {pipe} from 'fp-ts/function';

type MyError = NetworkError | BadUrl | BadResponse; // old types, implement them as you like

const toMyError = (e: Err): MyError => {
  swith(e.type) {
    case 'RequestError':
      return ... // NetworkError;

    case 'ResponseError': 
      return e.response.status === 404
                ? ... // BadUrl
                : ... // ResponseError
  }
}

declare const request: Req<unknown>

const myErrorRequest: TE.TaskEither<MyError, Resp<unknown>> = pipe(request(), TE.mapLeft(toMyError))

Please refer to #298 for further information

tmueller commented 2 years ago

Hi @StefanoMagrassi,

thanks for such a detailed answer. Was just wondering whether it caused problems, but as you answered, it didn't. Simplifying your API surface also is a good thing 👍. And you are right, current implementation leaves room for custom error handling.

Hope you don't mind me having another question. In request.ts the response body is consumed by calling text() to obtain data. Afterwards it's not possbile to use response.blob(), response.arrayBuffer() and the like, because the response body has already been consumed. Any idea how I could get Blobs or ArrayBuffers out of appys response?

StefanoMagrassi commented 2 years ago

@tmueller with the current implementation you cannot directly consume response data as blob o buffer (as you rightly pointed out, the response is always read as simple string).

You could use the cloneResponse utility and then read the response as blob or buffer from it, but I don't think it could be so useful...

We decided to always consume the response as string because covers most of the cases (appy was born as a library to fetch RESTfull API endpoints) and let us to easly introduce combinators like withDecoder

If you need some kind of "streaming" maybe this library is not the right choice (at the moment I can't imagine a solution to support also blobs and buffers)

tmueller commented 2 years ago

@StefanoMagrassi Yes I understand that consuming the body as text covers nearly all of the use cases and doing so reduces the steps to get ones data. Hence appy is very comfortable to use 👍

Thanks a lot for taking the time to answer.

A possible solution might be to export some kind of request(...) builder. Something like

type BuiltinFetch = typeof fetch;

export const taskifyFetch = (fetchP: BuiltinFetch) => TE.tryCatchK(
    fetchP,
    // logic for toResponseError or toRequestError
);

But this might be very well out of scope for appy.

StefanoMagrassi commented 2 years ago

yep, it was an idea I thought about it some months ago, but I haven't had time to work on it seriously

StefanoMagrassi commented 1 year ago

@tmueller I jumped back here for other reasons, but regarding this issue:

Hope you don't mind me having another question. In request.ts the response body is consumed by calling text() to obtain data. Afterwards it's not possbile to use response.blob(), response.arrayBuffer() and the like, because the response body has already been consumed. Any idea how I could get Blobs or ArrayBuffers out of appys response?

I would point out to you that we add this feature in v5.1.0 via requestAs() function (more on this in #665 and related PR #666)

tmueller commented 1 year ago

Thanks for the heads up and also thanks for implementing my suggestion in an easy to use manner. I'll try out and provide any feedback if needed. Keep up the fantastic work!