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

@:await JS Promises #46

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

@:await JS Promises #46

wants to merge 3 commits into from

Conversation

piboistudios
Copy link

This will just extract the T from js.lib.Promise<T> (if it is a js.lib.Promise) and wrap the expression in a check-type statement:
($expr : tink.core.Promise<T>).

@piboistudios piboistudios changed the title Convert @:await JS Promises @:await JS Promises Dec 24, 2021
@kevinresol
Copy link
Member

I prefer having the user manually covert it via Promised.ofJsPromise

@piboistudios
Copy link
Author

piboistudios commented Dec 24, 2021

I prefer having the user manually covert it via Promised.ofJsPromise

You should see my edit of the README ;)

Plus this wouldn't be a breaking change (as, this would have caused a compile-time error previously)

It's entirely optional

@kevinresol
Copy link
Member

I don't use tink_await much nowadays and I am not sure if it is good to lift everything to a Promise implicitly. I will let others to comment.

@Pign
Copy link

Pign commented Jun 17, 2022

@kevinresol I am not sure what your concern is? Do you want to avoid wrapping everything? I believe it is up to the dev to decide : they can either use @await and in which case it gets wrapped or if they do not want to wrap it they can just use .then on the object :)

@kevinresol
Copy link
Member

I main concern is the scope of this library. I am just not sure if we should include JS promises into our scope.

@Pign
Copy link

Pign commented Jun 17, 2022

Well I'd say that unification of promises to "the tink way" in the scope of a "tink framework" seems fair.

@benmerckx
Copy link
Member

benmerckx commented Jun 17, 2022

I main concern is the scope of this library. I am just not sure if we should include JS promises into our scope.

I can see how the earlier edits raised some concern. But the code change now is just applying tink.core.Promise.lift to the value that needs awaiting. Which also allows any abstract X to Promise to be awaited and that is pretty nice.

@kevinresol
Copy link
Member

the code change now is just applying tink.core.Promise.lift

Oh completely missed that part. well yeah then I agree this is good.

@benmerckx
Copy link
Member

@piboistudios could you reword the doc update a little to reflect that we simply use Promise.lift as opposed to extracting things?

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