Open gcanti opened 4 years ago
Thank you @gcanti for this proposal! I will play with your approach a bit and see if it suits my initial design goals.
I was thinking of the following scenarios when designed this package:
Programmer knows exactly all expected status codes – i.e., 200 | 400 | 401 | 403, – as well as payload type per each status. Then he/she can specify which statuses should be handled explicitly using .handle(code, handler, codec)
– in such case handler
should be inferred as a function receiving the same exact type of payload which was specified for the handled code
.
Programmer can handle explicitly not every HTTP code if he/she cares only about a subset – say, only 200 and 204, and for any other codes some kind of fallback should be executed. In this case programmer just calls:
fetcher
.handle(200, cb1, codec1)
.handle(204, cb2, codec2)
.discardRest(() => something)
As far as I can see right now, your approach requires full coverage of possible status codes in make
. I will explore if I can make your handlers
parameter a Partial<Record<S, Decoder<E, A>>>
, and make onUnexpectedError
cover the rest cases.
Another approach I was thinking of – make Fetcher
a set of classes, each with different capabilities. Something similar to a Free monads or Scala's ZIO design. Such approach allows even greater API discoverability – e.g., .handle()
could return a class which only has .handle
and .discardRest
methods, and .discardRest
returns a class which only has .run
method.
I was playing a bit with similar approach when explored possibility of session types implementation in TS: https://gist.github.com/YBogomolov/c5675b788a7046b6edf9e76ef7337af7. I really like the idea of discoverable APIs, when the compiler guides you towards correct consumption of the API.
As far as I can see right now, your approach requires full coverage of possible status codes
You can handle as many status codes as you wish (full or partial coverage)
//
// README example
//
import * as t from 'io-ts'
const User = t.type({ name: t.string })
const Users = t.array(User)
const FourTwoTwo = t.type({ code: t.number, correlationId: t.string })
interface User extends t.TypeOf<typeof User> {}
interface FourTwoTwo extends t.TypeOf<typeof FourTwoTwo> {}
type GetUserResult =
| { code: 200; payload: User[] }
| { code: 400; payload: Error }
| { code: 401; payload: [Error, string] }
| { code: 422; payload: FourTwoTwo }
// v-- ensures total coverage missing handlers ---v
const fetcher1: Fetcher<GetUserResult['code'], string, GetUserResult> = make('myurl', {}, () =>
E.left<string, GetUserResult>('unexpected error')
)
// ^--- error: Type 'Fetcher<never, string, GetUserResult>' is not assignable to type 'Fetcher<200 | 400 | 401 | 422, string, GetUserResult>'
import { flow } from 'fp-ts/lib/function'
import { failure } from 'io-ts/lib/PathReporter'
const decodeError = (errors: t.Errors): string => failure(errors).join('\n')
const handleUsers = flow(
Users.decode,
E.bimap(decodeError, payload => ({ code: 200 as const, payload }))
)
const handleFourTwoTwo = flow(
FourTwoTwo.decode,
E.bimap(decodeError, payload => ({ code: 422 as const, payload }))
)
// partial coverage, fetcher2 is inferred as: Fetcher<200 | 422, string, GetUserResult>
const fetcher2 = make(
'myurl',
{
200: handleUsers,
422: handleFourTwoTwo
},
() => E.left<string, GetUserResult>('unexpected error')
)
// you could also specify which statuses should be handled explicitly using
// a type annotation
const fetcher3 = make<200 | 422 | 400, string, GetUserResult>(
'myurl',
{
200: handleUsers,
422: handleFourTwoTwo
},
() => E.left('unexpected error')
)
// ^--- error: Property '400' is missing in type ...
Such approach allows even greater API discoverability
I agree, chainable APIs are nice for API discoverability, but there's no need for better discoverability with an API like make
: configuration is done in one shot.
Isn't the discardRest
signature problematic?
discardRest(restHandler: () => To)
What if I can't produce a To
(say I'm fetching a User
)?
p.s.
Btw I'm not sure about onUnexpectedError
's signature, perhaps should simply be (error: FetcherError) => E
Isn't the discardRest signature problematic? What if I can't produce a To (say I'm fetching a User)?
I though of restricting To
to have a monoid instance, so its unit could be returned in case of unexpected errors, making discardRest
not required. But on the second thought I decided that providing a fallback is easier than constructing a monoid instance.
@gcanti I would like to thank you once again for your feedback and suggested implementation. I played with it a little bit, and written the following: https://gist.github.com/YBogomolov/5de9290f6a7056f17a52b088ac298441
Main ideas:
Response
to the handlers.Decorder<E, A>
is ReaderTaskEither<Response, E, A>
now, as well as onUnexpectedError
is ReaderTaskEither<FetcherError, E, A>
.extend
, which adds new handlers to an existing fetcher (and updates its type). This allowed me to fulfil one of my initial design goals (which is implemented in current version of Fetcher
class): user should be able to gradually refine type of Fetcher
. This approach allows passing around partially built fetchers – say, in some "core" module you tell fetcher how to handle 400/401/403/500 error codes, and then in "app" module you tell it how to handle 200 code. In one of my commercial projects we used such approach and it proved its usefulness.I would be glad if you take a look at it 🙏
As handlers now work with a raw response, the
Decorder<E, A>
isReaderTaskEither<Response, E, A>
:+1: much better
onUnexpectedError is ReaderTaskEither<FetcherError, E, A>
Looks like JsonDeserializationError
is unused now, what about onUnexpectedError: Decoder<E, A>
?
export interface Fetcher<S extends Status, E, A> {
...
readonly onUnexpectedError: Decoder<E, A>;
...
}
(maybe should be renamed)
Decoders for text & JSON could be exposed by default
:+1:
This approach allows passing around partially built fetchers
I'd say you pass around partially built handlers
export type Handlers<S extends Status, E, A> = Record<S, Decoder<E, A>>
// partially built handlers
const handlers: Handlers<400 | 100, Error, never> = {
400: () => TE.left(new Error('400')),
100: () => TE.left(new Error('100'))
}
const fetcher21 = extend(fetcher2, handlers)
Nitpick: now handlers can be overwritten, the name "extend" could be misleading, what about
// disallow overwrites ---v
export function extend<OldS extends Status, NewS extends Exclude<Status, OldS>, E, A>(
fetcher: Fetcher<OldS, E, A>,
handlers: Record<NewS, Decoder<E, A>>,
onUnexpectedError?: ReaderTaskEither<FetcherError, E, A>
): Fetcher<OldS | NewS, E, A>
@gcanti it really clicks now :) I've split extend
into two implementations: extendWith
, which allows changing type of the result (A => B
), and extend
, which acts as an endomorphism on fetcher's result type. And the implementation is much clearer. Please see this iteration: https://gist.github.com/YBogomolov/5de9290f6a7056f17a52b088ac298441
Nitpick: now handlers can be overwritten, the name "extend" could be misleading, what about
I totally forgot about the Exclude
, shame on me. Thanks for the tip!
(maybe should be renamed)
I don't like the long name, but it's quite verbose, so I'd stick with it ¯_(ツ)_/¯
which allows changing type of the result (A => B)
Isn't this map
?
export function map<S extends Status, E, A, B>(fetcher: Fetcher<S, E, A>, f: (a: A) => B): Fetcher<S, E, B> {
return {
input: fetcher.input,
handlers: pipe(fetcher.handlers, R.map(RTE.map(f))) as Record<S, Decoder<E, B>>,
onUnexpectedError: pipe(fetcher.onUnexpectedError, RTE.map(f)),
init: fetcher.init
}
}
// not interesting since can be derived from `map` and `extend`
export const extendWith = <A, B extends A>(ab: (a: A) => B) => <
OldS extends Status,
NewS extends Exclude<Status, OldS>,
E
>(
fetcher: Fetcher<OldS, E, A>,
handlers: Handlers<NewS, E, B>
): Fetcher<OldS | NewS, E, B> => extend(map(fetcher, ab), handlers)
Isn't this map?
Yep, it is. I'll add map
/mapLeft
/bimap
, they will be handy.
// not interesting since can be derived from
map
andextend
And extend
could be expressed as extendWith(identity)
🙃 Still it makes API a bit more welcoming, IMHO.
I will test this vnext
iteration of fetcher-ts for a bit and see how it feels from the point of end-user. Once again – thank you @gcanti for your collaboration on this project! The design you've suggested seems more streamlined with pipeable
approach. I really like it.
The resulting iteration: https://gist.github.com/YBogomolov/5de9290f6a7056f17a52b088ac298441
This is a great thread! I've been thinking about a fetcher that could take a set of decoders as input in form of a tagged union (here and here) but never got to realize my ambition 😅 . I like the change that made onUnexpectedError
a required param. There's always something unexpected that should be taken care of 🙂 That brings me to my question. If I were to implement a circuit breaker on top of this new API, how would I go about it? The circuit breaker should be configurable per client which might translate to per "instance" of a fetcher. Furthermore, the configuration should contain a threshold and a list of "exceptions" which can be response status codes, kinds of error (network timeout error, socket error, connection error, etc.). To achieve this, the "left" type should provide data that allows these cases to be identified. The circuit breaking is just an example, one might want to wrap the fetcher with a simple retry logic that also depends on the response status code because there's no point in retrying if the error is a client error.
Bonus question: would this fetcher work with the experimental modules of io-ts?
Based on the following (self imposed) requirements:
io-ts
cross-fetch
E.right
)I would propose an API like this:
Example