-
Notifications
You must be signed in to change notification settings - Fork 71
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
Comments
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 |
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 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
I'm sure I'm missing other benefits! What others would you add to the list? |
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:
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? |
I haven't heard anything from consumers about |
I've been encountering the same pains that the async variant solves. |
@meh the ts-async-results is in prod for a while now. I'd be curious to hear if it solves your issues! |
@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. |
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 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!')); |
Good example @ScottLindley. The ts-async-results API currently supports pretty much all of that. Let me know if that works for you. |
@GabrielCTroia @vultix any update on whether you'd like to move forward with an effort to include |
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
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:
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 I think it's more a matter of wanting them to be married so I don't need two separate dependencies. |
@ScottLindley I feel you! I'm can't make the calls on that! |
@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 |
And @GabrielCTroia some quick feedback on the 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..) |
Hey all, just FYI I've recently added an initial async support to our The initial support consists of an async version of 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 |
@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. |
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 :)
The text was updated successfully, but these errors were encountered: