Skip to content
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

Asynchronicity #18

Open
GabrielCTroia opened this issue Jan 12, 2021 · 19 comments
Open

Asynchronicity #18

GabrielCTroia opened this issue Jan 12, 2021 · 19 comments

Comments

@GabrielCTroia
Copy link

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 :)

@vultix
Copy link
Owner

vultix commented Jan 12, 2021

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

@GabrielCTroia
Copy link
Author

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.

@vultix
Copy link
Owner

vultix commented Jan 17, 2021

@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?

@GabrielCTroia
Copy link
Author

GabrielCTroia commented Jan 18, 2021

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?

@vultix
Copy link
Owner

vultix commented Jan 18, 2021

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.

@meh
Copy link

meh commented May 11, 2021

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

@GabrielCTroia
Copy link
Author

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

@vultix
Copy link
Owner

vultix commented May 23, 2021

@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.

@ScottLindley
Copy link

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!'));

@GabrielCTroia
Copy link
Author

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

@ScottLindley
Copy link

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

@GabrielCTroia
Copy link
Author

GabrielCTroia commented Feb 2, 2022

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".

@ScottLindley
Copy link

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

@GabrielCTroia
Copy link
Author

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

@skoshx
Copy link

skoshx commented Dec 4, 2023

@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
Copy link

skoshx commented Dec 4, 2023

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.

@GabrielCTroia
Copy link
Author

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..)

@jstasiak
Copy link
Contributor

jstasiak commented Dec 5, 2023

Hey all, just FYI I've recently added an initial async support to our ts-results fork (ts-results-es): lune-climate#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().

@skoshx
Copy link

skoshx commented Dec 6, 2023

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants