gigobyte / purify

Functional programming library for TypeScript - https://gigobyte.github.io/purify/
ISC License
1.53k stars 59 forks source link

Add Either API methods to EitherAsync #183

Closed gigobyte closed 4 years ago

gigobyte commented 4 years ago

Will resolve to a promise.

EDIT:

alt
(other: EitherAsync<L, R>): EitherAsync<L, R>

orDefault
(defaultValue: R): Promise<R>

leftOrDefault
(defaultValue: L): Promise<L>
chenguo commented 4 years ago

Can you please elaborate on this ticket, what do you mean by "resolve to a promise"? Are you changing the run() call?

We're trying out your lib in our code base, and it became painful when we got to the async part where some Either become Promise<Either>s. Can you talk a bit about your thoughts on where you want to take this interface?

gigobyte commented 4 years ago

Are you changing the run() call?

No, the API will remain the same, but I want to remove some of the boilerplate.

I noticed a pattern in my codebase, when I want to do something with the result it often ends up like this:

EitherAsync(() => {
    // ...stuff
})
  .run()
  .then(eitherSomething => {
       eitherSomething.ifRight(something => {
         // send a response in express
         res.json(something )
      })
  })

I plan to add most of the normal Either methods to EitherAsync (and same for Maybe/MaybeAsync) so that the code above will look like this:

EitherAsync(() => {
    // ...stuff
})
  .ifRight(something => {
       res.json(something)
  })

EDIT: To clarify, "Will resolve to a promise" means that methods like caseOf, orDefault etc will return a Promise as it doesn't make any sense to run orDefault and get back an Either again, so they will run the promise inside the EitherAsync.

// Before
const result: Promise<string> =
   EitherAsync().run().then(e => e.orDefault(''))

// After
const result: Promise<string> =
   EitherAsync().orDefault('')

Ideally using run should be an edge case.

gigobyte commented 4 years ago

While also writing a DB handler for an application of mine, I needed a Promise.finally equivalent for EitherAsync and there isn't any, so that will be added as well. I'm trying to make EitherAsync kind of a drop-in replacement for Promise, to provide the same functionality at the least.

chenguo commented 4 years ago

Gotcha, yeah this seems like it'd be helpful.

If you can make make Eithers and EitherAsync have the same API, can we have something like an IEither (better named of course) where Either functions that return an Either can return an IEither instead?

A point of specific pain I've come upon is in chaining Eithers with an async function, I've had to resort to something like following quite often to bridge this gap, where I'm forced to use a caseOf because it's not opinionated about its return type:

function foo(
  aRegularEither: Either<string, number>
): Promise<Either<string, nmber>> {
  return aRegularEither.caseOf({
    Left: identity,
    Right: (value) => doAsyncThing(value)
 });
}

In isolation this isn't a big deal, but when I want to chain a few monadic operations together, having to break the chain in the middle with a call to EitherAsync.liftEither() interrupts the flow and makes things generally harder to read.

I haven't fully fleshed out my thoughts here, so this is largely spitballing. I'll have to do some more thinking to tie things together. One random observation that may be helpful: I remember seeing some HTTP library (I think superagent) where they defined Request extends Promise<Response>, this may be at least something to think about, since it would be convenient for EitherAsync and Either to have a similar relationship where if you await an EitherAsync you get an Either.

chenguo commented 4 years ago

I read through the code more deeply, so it looks like Either is already an interface, and if all the functions make it over to EitherAsync then chaining together asynchronous eithers should be pretty easy.

A question about EitherAsyncImpl, why is the promise mechanism here so abstract? I'm talking about this chunk of code below:

  constructor(
    private runPromise: (helpers: EitherAsyncHelpers<L>) => PromiseLike<R>
  ) {}

  async run(): Promise<Either<L, R>> {
    try {
      return Right(await this.runPromise(helpers))
    } catch (e) {
      return Left(e)
    }
  }

Why not have the constructor take an already created Promise<Either<L, R>>, to make this class more flexible? I'm curious why the coupling between helpers and EitherAsyncImpl is necessary here.

Another one: tell me what you think about this: https://github.com/chenguo/purify/pull/1 This just makes it possible to await an EitherAsync. I'm working in a codebase that heavily uses awaits for async code and I feel I'd have an easier time convincing people to accept Eithers if we can make the simplest interface possible (i.e. not introduce the logical complexity of a separate run() function)

KevinMGranger commented 4 years ago

Why not have the constructor take an already created Promise<Either<L, R>>, to make this class more flexible?

Most things that produce a Promise immedately start executing them, while EitherAsync is supposed to represent a deferred computation. If you really do want to use an existing promise, changing EitherAsync(promise) to EitherAsync(() => promise) is only 5 additional characters.

chenguo commented 4 years ago

Most things that produce a Promise immedately start executing them, while EitherAsync is supposed to represent a deferred computation.

Gotcha, that makes perfect sense.

Here's a couple things I'm playing with. Say I have this function:

async function asyncThing(n: number): Promise<number> {
  return n + 1;
}

It's a very common pattern to want to do something like:

const n: Either<string, number> = Right(10);
const result = liftEither(n)
  .map(asyncThing);

But now result has the type EitherAsync<never, Promise<number>>, whereas it makes sense to flatten the promises and produce a EitherAsync<never, number>. I found this can actually be trivially accomplished just by changing the signature of map so Typescript understands whats going on:

before:

  map<R2>(f: (value: R) => R2): EitherAsync<L, R2>

after (apply to both interface EitherAsync and class EitherAsyncImpl)

  map<R2>(f: (value: R) => R2 | PromiseLike<R2>): EitherAsync<L, R2>

I also want to have chain working seamlessly with eithers and async eithers. Say I have:

function eitherThing(n: number): Either<string, number> {
  return Right(n + 1);
}

async function asyncEitherThing(n: number): Promise<Either<string, number>> {
  return eitherThing(n);
}

So if I wanted to chain this I'd have to do something like:

  const n: Either<string, number> = Right(10);
  const result = liftEither(n)
    .map(asyncThing)
    .chain(value => liftEither(eitherThing(value)))
    .chain(value => liftPromise(() => asyncEitherThing(value))); // I think fromPromise also works here

But what I really want to do is:

  const n: Either<string, number> = Right(10);
  const result = liftEither(n)
    .map(asyncThing)
    .chain(eitherThing)
    .chain(asyncEitherThing);

On my fork I have a PR where I made EitherAsync extend Promise<Either>, and with that it actually just took declaring an interface and adding a Promise.resolve to EitherAsync.chain to get it all working together. My fork's branch is updated with the following:

// I'll come up with a better name for this:
export type EitherAsyncable<L, R> = Either<L, R> | EitherAsync<L, R> | Promise<Either<L, R>>;

// new chain interface and implementation:
  chain<R2>(f: (value: R) => EitherAsyncable<L, R2>): EitherAsync<L, R2> {
    return EitherAsync(async (helpers) => {
      const value = await this.runPromise(helpers)
      return helpers.fromPromise(Promise.resolve(f(value)))
    })
  }

If this is a direction we want to go in (god please say yes) I'd love to flesh this out to the rest of the interface functions and make an official PR to Purify.

chenguo commented 4 years ago

Heh with map and chain I was halfway to updating the whole EitherAsync interface, so I just went ahead and extended mapLeft and chainLeft as well.

PR here: https://github.com/gigobyte/purify/pull/192

None of the existing tests are affected. I added tests for each interface I touched. Please let me know if there's any concerns / changes you want, I'd love to get some form of this imerged so my I can expand Purify-TS to more of my project.

gigobyte commented 4 years ago

Related issue: https://github.com/gigobyte/purify/issues/180

These suggestions are definitely interesting.

On an unrelated note, I'm scared of drastically changing the Async API, a lot of people are already depending on it. In hindsight, I should've marked it as WIP or Beta to let people know that future breaking changes are possible. I definitely didn't expect so much attention to this module, it started as an experimented and I expected to flop immediately after release. It shows in the initial design that I didn't really know what my goal was, but I'm happy that with time the API is looking better and better and we are ironing issues out.

chenguo commented 4 years ago

On an unrelated note, I'm scared of drastically changing the Async API, a lot of people are already depending on it.

FWIW none of the existing unit tests broke with these changes :)

jbuijgers commented 4 years ago

This library has become part of (almost) all the code I'm currently writing, and I can strongly relate to the use-case @chenguo is describing. I personally would have no trouble at all changing all my code to support this. Since we are still at version 0.x.x, I don't mind drastic API changes to get this library to move forward. Thanks for all the great work!

gigobyte commented 4 years ago

Implemented in https://github.com/gigobyte/purify/pull/247