-
Notifications
You must be signed in to change notification settings - Fork 6
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
Roll back changes to TaskEither#tryCatch #50
Comments
I went ahead and quickly implemented the proposed style for TaskEither with a strictly sync
It's great that we're achieving these goals, which we did not before. The new style yields better to a point-free composition. const getDbById = flow(
tryPromise(
databases.retrieve,
(_, { database_id }) => new Error(`Couldnt fetch db: ${database_id}`)
),
map(doWork),
fold(onLeft, onRight)
)
type GetDbById = ( The OG tryCatch implementation only worked if you built a pipeline, that's pretty awesome! How about?We went in the direction of The trick here is that export function tryCatch<AS extends unknown[], A, B>(
fasr: (...as: AS) => A,
onThrow: (e: unknown, ...as: AS) => B,
): (...as: AS) => TaskEither<B, A> {
return (...as) => {
const _onThrow = (e: unknown) => eitherLeft(onThrow(e, ...as));
return handleThrow(
() => fasr(...as),
(r) => Promise.resolve(r).then(eitherRight).catch(_onThrow),
(e) => Promise.resolve(_onThrow(e)),
);
};
} This implementation to me passes the following suite: Deno.test("TaskEither tryCatch", async (t) => {
const expectedRight = 2;
const expectedLeft = "Bad";
const throws = () => {
throw new Error("Boom");
};
await t.step("Sync flow", async () => {
await assertEqualsT(
T.tryCatch(throws, () => expectedLeft)(),
T.left(expectedLeft),
);
await assertEqualsT(
T.tryCatch((n: number) => n * 2, String)(1),
T.right(expectedRight),
);
await assertEqualsT(
pipe(T.tryCatch(() => 1, String)(), T.map((n) => n * 2)),
T.right(expectedRight),
);
});
await t.step("Async flow", async () => {
await assertEqualsT(
T.tryPromise((n: number) => resolve(n * 2), String)(1),
T.right(expectedRight),
);
await assertEqualsT(
T.tryPromise(resolve, String)("That's right."),
T.right("That's right."),
);
await assertEqualsT(
T.tryPromise(
(..._args: [string, ...string[]]) => throws(),
(_, as) => as.join(" "),
)("Pass", "all", "them", "args"),
T.left("Pass all them args"),
);
await assertEqualsT(
T.tryPromise(throws, () => expectedLeft)(),
T.left(expectedLeft),
);
});
}); |
My initial feeling is to be against a magical lifting into promise. But seeing as this will only apply to the Task, TaskEither, and Affect ADTs I think it might make sense. It does seem appealing to not have to think about whether one is lifting an async or sync function into Task/TaskEither and it has been nice to use in practice too. I think it's a great idea. I'll try to poke holes in it this week and if I can't come up with anything I think we'll go with your joined implementation for Task/TaskEither. I am also in the middle of reconsidering the interface for Affect and whether it should be: export type Affect<C extends unknown[], B, A> = (...c: C) => Promise<Either<B, A>> instead of: export type Affect<C, B, A> = Reader<C, Promise<Either<B, A>>>; I'm going to setup a new trial Affect implementation with multivariate functions like this to see how it looks and acts. |
@pixeleet It seems like your implementation has a type error. For a type JoinPromise<A> = A extends Promise<infer I> ? I : A;
export function tryCatch<AS extends unknown[], A, B>(
fasr: (...as: AS) => A,
onThrow: (e: unknown, ...as: AS) => B,
): (...as: AS) => TaskEither<B, JoinPromise<A>> {
return (...as) => {
const _onThrow = (e: unknown) => eitherLeft(onThrow(e, ...as));
return handleThrow(
() => fasr(...as),
(r) => Promise.resolve(r).then(eitherRight).catch(_onThrow),
(e) => Promise.resolve(_onThrow(e)),
);
};
} |
Regarding Affect. Was wondering if we're supposed to execute it straight up on receiving I know you went for this implementation originally because you dislike the Also am wondering what's wrong with the following snippet (that imho should work) import { assertEquals } from "https://deno.land/std/testing/asserts.ts";
import * as A from "./affect.ts";
import { identity, pipe } from "./fns.ts";
const aff1 = pipe(A.ask<string>(), A.map((a) => a.toUpperCase()));
const aff2 = A.of<string>("b");
const add = ({ a, b }: { a: string; b: string }) => a + b;
const x = pipe(
aff1,
A.bindTo("a"),
A.bind("b", () => aff2),
A.map(add),
A.fold((_) => "aa", identity),
)("a");
const y = pipe(
A.Do(),
A.bind("a", () => aff1),
A.bind("b", () => aff2),
A.map(add),
A.fold((_) => "aa", identity),
)("a");
assertEquals(await x, "Ab");
assertEquals(await y, "Ab"); Only after resorting to use any a second type parameter this starts to work correctly. import { assertEquals } from "https://deno.land/std/testing/asserts.ts";
import * as A from "./affect.ts";
import { identity, pipe } from "./fns.ts";
const aff1 = pipe(A.ask<string, any>(), A.map((a) => a.toUpperCase()));
const aff2 = A.of<string, any, string>("b");
const add = ({ a, b }: { a: string; b: string }) => a + b;
const x = pipe(
aff1,
A.bindTo("a"),
A.bind("b", () => aff2),
A.map(add),
A.fold((_) => "aa", identity),
)("a");
const y = pipe(
A.Do(),
A.bind("a", () => aff1),
A.bind("b", () => aff2),
A.map(add),
A.fold((_) => "aa", identity),
)("a");
assertEquals(await x, "Ab");
assertEquals(await y, "Ab"); What am I doing wrong? |
Good catch! That's because This should fix it: export function tryCatch<AS extends unknown[], A, B>(
fasr: (...as: AS) => A | PromiseLike<A>,
onThrow: (e: unknown, ...as: AS) => B,
): (...as: AS) => TaskEither<B, A> {
return (...as) => {
const _onThrow = (e: unknown) => eitherLeft(onThrow(e, ...as));
return handleThrow(
() => fasr(...as),
(r) => Promise.resolve(r).then(eitherRight).catch(_onThrow),
(e) => Promise.resolve(_onThrow(e)),
);
};
} |
Catching this should be the job of type level testing. I wonder if you had any exposure to it and have an approach in mind? |
Well, I never really tried using Do notation for Affect, this is likely the first problem. I don't have much time right now, but for the x example I see the following hints from the first code block:
Looks like the type for |
In #31 we changed tryCatch for taskEither to:
While working on #39 I finally remembered why I was against this change. The simple answer is that
Task
is never supposed to throw. None of the Task combinators handle the thrown side of the returned promise, which makes it exactly the wrong vehicle to wrap promise thunks that can throw. In actuality, a Task that can fail is no Task at all. While there may be functions in typescript with the type() => Promise<A>
, if these functions can throw or their promises can throw then they shouldn't be Tasks.First, let's disambiguate the various cases where we handle untyped thrown errors:
type Function<AS extends unknown[], R> = (...as: AS) => R
type AsyncFunction<AS extends unknown[], R> = (...as: AS) => Promise<R>
We will want to define tryCatch-like functions for many of the ADTs in fun. Following is a list of the ADTs in fun where it makes sense to implement tryCatch. ADTs that should also handle async tryCatch will be marked with
tryPromise
:Here, a * denotes that the non-thunk versions of tryCatch can cause some issues. Lets look at those (Task, TaskEither).
When implementing tryCatch for Task there is a natural implementation:
But what if we want to turn a non-thunk into a Task? Then we might have:
Both of these are valid cases, but the second one covers the usage of the first as well, but ends up returning
Task<Task<R>>
when the function is a thunk. These cases repeat for promise returning functions:After writing these implementations and many others I realized why this problem was likely seeming more complicated than it is. The example used as the reason for this change was this:
Here the old way was problematic because it was trying to use tryCatch (which only accepted thunks) to map a promise returning function to TaskEither. If we had restated the question to: What is the most sensible way to take functions of the form
type Input<AS extends unknown[], R> = (...as: AS) => Promise<R>
and turn them into ADTs like Task, TaskEither, and Affect? Well, lets list the things we want tryCatch to do:Because of 1 I propose that idiomatic tryCatch shouldn't default to thunks. This means we would move away from
taskTryCatch1
style implementations and towardstaskTryCatch2
.Because of 2 I propose that we break tryCatch into a synchronous tryCatch version and an async tryPromise version. This is to avoid testing for Promises in both the result and throw cases of tryCatch.
Because of 4 the onThrow case should accept both the error and the arguments array. Here we have the choice of 1
(e: unknown, ...as: AS)
or 2(e: unknown, as: AS)
. For consistency reasons (and to cut down on an extra spread operation) I am tentatively going with 2.This makes our implentations for Task and TaskEither as follows:
and the usage of the original case looks something like this:
The text was updated successfully, but these errors were encountered: