ts-results icon indicating copy to clipboard operation
ts-results copied to clipboard

Asynchronicity

Open GabrielCTroia opened this issue 4 years ago • 19 comments

Hey @vultix – I just published ts-async-results, as I found myself often having to use ts-results in async contexts and I found it very cumbersome to use it as Promise<Result<SuccessType, ErrorType>>.

As it's hugely influenced by your work here, your feedback would be very important to me and others, so please take a look and maybe even give it a try!

Also, sorry for the weird way of connecting with you via the Github Issues, but I had no other means :)

GabrielCTroia avatar Jan 12 '21 05:01 GabrielCTroia

I’ll likely not get to this until the weekend, but I’ll love to take a look!

If it will make things easier, I’m open to the idea of adding async results into this library as well (crediting you). I don’t mean to steal your work, so if you’d rather it stay separate I’ll happily point people to your library

vultix avatar Jan 12 '21 05:01 vultix

Hey @vultix, perfect – whenever you have time! Thank you!

Oh yeah for sure! If it makes sense to bundle them together I'd be more than happy to – since it's going to be easier for the community to adopt it too most likely, but we should "sit down" one day and go over how we'd do it and what some pros/cons are to that.

GabrielCTroia avatar Jan 12 '21 19:01 GabrielCTroia

@GabrielCTroia I just gave your code a once-over, and it looks really nice! These are the benefits I see to using your library (over just using Promise<Result<T, E>>):

  • Less verbose to type AsyncResult<T, E>
  • You can call result operations like mapErr directly before awaiting:
    • From this: (await getPromiseResult()).map(handleOk).mapErr(handleErr)
    • To this: await AsyncResultWrapper(getPromiseResult()).map(handleOk).mapErr(handleErr).resolve()

I'm sure I'm missing other benefits! What others would you add to the list?

vultix avatar Jan 17 '21 22:01 vultix

Hey @vultix. Thank you!

Yes you're right, those are the benefits! There isn't really much else, at least for now.

Also, just to note a couple of things:

  1. It's meant to integrate with ts-result out of the box. It does so for most parts already but there's room for improvement.
  2. It's not fully on Par with ts-result's API (yet) but it should get there.

So yeah, given the 2 above, it would make sense to bundle them under one project.

Have you heard anything from ts-results's consumers of a need for better Async support so far?

GabrielCTroia avatar Jan 18 '21 00:01 GabrielCTroia

I haven't heard anything from consumers about ts-results being difficult to use in async contexts, and I've not used it much with async myself. I'm super interested to hear what problems you have to see if we can address them.

vultix avatar Jan 18 '21 22:01 vultix

I've been encountering the same pains that the async variant solves.

meh avatar May 11 '21 13:05 meh

@meh the ts-async-results is in prod for a while now. I'd be curious to hear if it solves your issues!

GabrielCTroia avatar May 16 '21 20:05 GabrielCTroia

@meh @GabrielCTroia Can either of you provide a few examples of the pains you're running into working with async / await. I'd love to find a solution for this, but want to see how this is affecting users so we can come up with a great api.

vultix avatar May 23 '21 17:05 vultix

I can also raise my hand and say it's very clumsy to work with promises currently. I think mainly the pain is that once you go async, each step beyond that requires that you await or use .then on the current promise to access the Result type underneath. This means that you don't have a unified result/promise chain that can read in a linear manner. Here's a really contrived example:

const doSyncThing => Ok.EMPTY;
const doAsyncThing = async (): Promise<Result<string, never>> =>
  Ok('Did an async thing!');

// Current API
doSyncThing()
  .map(() => doAsyncThing())
  .map((result: Promise<Result<string, never>>) =>
    // currently I have to await this first because I get a `Promise<Ok<string>>`,
    // I'd _like_ this to just be of type `string`.
    result.then((value: Result<string, never>) =>
      // After awaiting the promise I still don't have my value but instead `Ok<string>`.
      value
        .map((okInnerValue: string) => okInnerValue.split(' '))
        .map((words: string[]) => words.reverse())
        .map((reversedWords: string[]) =>
          Promise.all(
            reversedWords.map(
              async (word): Promise<Result<string, never>> =>
                // Same issue, gotta do the whole await them map for these as
                doAsyncThing().then((result: Result<string, never>) =>
                  result.map(
                    (okInnerValue: string) => `${word} ${okInnerValue}`
                  )
                )
            )
          )
        )
        .map((p: Promise<Result<string, never>[]>) =>
          p.then((promiseResults: Result<string, never>[]) =>
            Result.all(...promiseResults).map((finalResult: string[]) =>
              console.log(finalResult)
            )
          )
        )
    )
  );

// Desired API
doSyncThing()
  .map(() => doAsyncThing())
  // Now I can map over the resolved value from the async operation.
  .map((resolvedValue: string) => resolvedValue.split(' '))
  .map((words: string[]) => words.reverse())
  .map((reversedWords) =>
    Result.all(
      reversedWords.map(async (word: string) =>
        doAsyncThing().map(
          (resolvedValue: string) => `${word} ${resolvedValue}`
        )
      )
    )
  )
  .map((finalResults: string[]) => console.log(finalResults))
  // Perhaps I can use `catch` here or something similar here so errors don't get swallowed.
  .catch((e) => console.log('unexpected error!'));

ScottLindley avatar May 31 '21 22:05 ScottLindley

Good example @ScottLindley. The ts-async-results API currently supports pretty much all of that. Let me know if that works for you.

GabrielCTroia avatar May 31 '21 22:05 GabrielCTroia

@GabrielCTroia @vultix any update on whether you'd like to move forward with an effort to include ts-async-results in this library?

ScottLindley avatar Feb 02 '22 00:02 ScottLindley

Hey @ScottLindley, I can't speak for ts-results but I'm curious if ts-async-results is still not on par with what you're looking for. If so please let us know what the exact shortcomings are?

Looking at your example

doSyncThing()
  .map(() => doAsyncThing())
  // Now I can map over the resolved value from the async operation.
  .map((resolvedValue: string) => resolvedValue.split(' '))

the limitation is that you cannot pipe from the sync ts-result to async ts-async-results out of the box, but all you actually need to do is wrap it in an asyncResult first like this:

new AsyncResultWrapper(doSyncThing())
  .map(() => doAsyncThing())
  // Now I can map over the resolved value from the async operation.
  .map((resolvedValue: string) => resolvedValue.split(' '))

That could be an improvement but imho the tradeoff for now isn't the worst. Let me know if I'm missing something!

Also, about the "catch" statement at the end, usually errors don't get swallowed if you use AsyncResults the proper way, they just get caught by "mapError".

GabrielCTroia avatar Feb 02 '22 16:02 GabrielCTroia

@GabrielCTroia I think it's more a matter of wanting them to be married so I don't need two separate dependencies.

ScottLindley avatar Feb 02 '22 16:02 ScottLindley

@ScottLindley I feel you! I'm can't make the calls on that!

GabrielCTroia avatar Feb 02 '22 17:02 GabrielCTroia

@vultix Friendly bump :) This would be something I would also love to see. For instance being able to do:

const requestResults = await Result.all(
  // these are async 
  fetchMyProjects,
  fetchMyTeams
)

const [myProjects, myTeams] = requestResults.val;
//     ^^^^^^^^^^^^^^^^^ even this kind of API would save many lines.

I think async/await fits perfectly with the Result paradigm, where rejections would be mapped as normal, and the developer could only return Ok & Err from Promises so that the typing works perfectly!

skoshx avatar Dec 04 '23 21:12 skoshx

And @GabrielCTroia some quick feedback on the ts-async-results, I think having to wrap almost everything with AsyncResultWrapper is already enough for me not to use ts-async-results.

Ideally the API would be completely similar, aka:

const fetchProfile = async () => {
  try {
     const profileJson = await (await fetch('../profile')).json()
     return Ok(profileJson)
  } catch (e) {
    return Err(e)
  }
}


Although I'm not 100% sure there are some JS tricks that you could utilize to get that working.

skoshx avatar Dec 04 '23 21:12 skoshx

And @GabrielCTroia some quick feedback on the ts-async-results, I think having to wrap almost everything with AsyncResultWrapper is already enough for me not to use ts-async-results.

Ideally the API would be completely similar, aka:

const fetchProfile = async () => {
  try {
     const profileJson = await (await fetch('../profile')).json()
     return Ok(profileJson)
  } catch (e) {
    return Err(e)
  }
}

Although I'm not 100% sure there are some JS tricks that you could utilize to get that working.

With this version you lose the API (map, mapErr, etc..)

GabrielCTroia avatar Dec 05 '23 00:12 GabrielCTroia

Hey all, just FYI I've recently added an initial async support to our ts-results fork (ts-results-es): https://github.com/lune-climate/ts-results-es/pull/87 released to NPM as 4.1.0-alpha.1 for testing.

The initial support consists of an async version of ResultAsyncResult – with a couple of methods (map(), andThen()) and ways to convert between Result and AsyncResult.

The documentation: https://ts-results-es.readthedocs.io/en/latest/reference/api/asyncresult.html

It's quite convenient I would say.

We'll be adding mapErr() and maybe a few other composition methods to AsyncResult and then also an async Option counterpart is needed (+ an async version of Result.all(), Result.any().

jstasiak avatar Dec 05 '23 08:12 jstasiak

@GabrielCTroia Right yeah, overlooked that completely haha. But yeah from my point aswell, would be great to have @vultix respond, or then give maintainer rights for someone else so that there wouldn't have to be multiple forks being maintained.

skoshx avatar Dec 06 '23 22:12 skoshx