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

Feature/async mappers Result & Maybe #179

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

ChrisHadar
Copy link

I've added methods to both Result and Maybe which will automatically unwrap and wrap Promises with more intuitive behavior when mapping async functions

@patrickmichalina
Copy link
Owner

@ChrisHadar Thanks. It is going to be a little bit before I fully review this. I am swamped at the moment and need to carve some time to think about this and the approach. Thanks!

@greym0uth
Copy link

greym0uth commented Feb 23, 2022

This looks really good to me: has tests, no breaking changes, and very useful. You think you would have time @patrickmichalina at some point to review this?

@patrickmichalina
Copy link
Owner

@greym0uth this PR contains async/await which is not something I would want in a lazily evaluated library. Because I see that I have not focused on getting this reviewed. But thanks for the reminder and I will try to pencil it in.

@greym0uth
Copy link

@patrickmichalina Ah I see yea, perhaps @ChrisHadar could change it to use promises instead of the async/await keywords then?

@greym0uth
Copy link

Also @patrickmichalina why don't you want async/await keywords? It's been a core feature of typescript since 2.1 and since these are TS files anything evaluating them should have async/await keyword support. Rollup can polyfill the build if it doesn't already (I think it translates to promises in the resulting JS already).

@vermiculus
Copy link

Hi folks -- is there anything I can do to help move this PR forward? We're at the point of potentially maintaining our own version of this package in order to support this syntax.

@patrickmichalina
Copy link
Owner

@vermiculus I will try get this over the fence

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

Successfully merging this pull request may close these issues.

4 participants