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

Add standard convenience functions for creating Promises to resolve groups of Futures. #1844

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

EliteMasterEric
Copy link
Contributor

Several convenience functions which mostly mimic promise handling functionality from JavaScript. Definitely standard enough to be part of the base library.

  • Promises.all() resolves an array of Futures, completing when all of the Promises are fulfilled or any of the Promises are rejected.
  • Promises.allSettled() resolves an array of Futures, completing when all of the Promises are fulfilled or rejected.
  • Promises.any() resolves an array of Futures, completing when any of the Promises is fulfilled or all of the Promises are rejected.
  • Promises.race() resolves an array of Futures, completing when any of the Promises is fulfilled or rejected.

Good example use cases include calling lime.utils.Assets.loadImage() for each of an Array of images; you now have multiple promises, and want to wait for all of them to resolve before continuing.

@nanjizal
Copy link

Not expert in this area, I wonder have you evaluated Franco's code in relation to this.
https://github.com/fponticelli/thx.promise
it maybe old in relation to some haxe features but generally he was advanced in his use of haxe back then, and he was certainly not limited to mirroring just one language.

@EliteMasterEric
Copy link
Contributor Author

Not expert in this area, I wonder have you evaluated Franco's code in relation to this.
https://github.com/fponticelli/thx.promise

thx.promise is not compatible with the lime.app.Promise and lime.app.Future classes, which are built into Lime and utilized by Lime and OpenFL's asynchronous asset fetching functions, among other things.

@nanjizal
Copy link

but you read understood and learned from Franco, or did you just consider if the code was compatible - because that would be disappointing. There are creatives you learn from and I suspect Franco for haxe is one of them, and if you learnt nothing from his work then your commit is probably shallow? Please take another look at franco's stuff and evolve.

@m0rkeulv
Copy link
Member

m0rkeulv commented Sep 29, 2024

Not sure why thx.promise would be relevant here, as far as i can tell this is just a few utility methods making existing lime code more convenient to work with.

i do wonder if the PromiseResult would be simpler as an Enum tho, just my two cents.

 enum PromiseResult<T> {
  Pending;
  Fulfilled(value:T);
  Rejected(error:Dynamic);
}

* A set of static utility functions for working with Promises.
* This includes functions which resolve groups of Futures.
*/
class Promises {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not add the functions to Promise?

src/lime/app/Promises.hx Outdated Show resolved Hide resolved
@EliteMasterEric
Copy link
Contributor Author

I just went to make some refinements to this PR, and tried to implement your suggestion to add the static functions to the Promise class instead of making a new Promises class.

However, I discovered that since Promise is an @:generic class, it does not support static variables or functions. Thus, I will have to revert it back.

Using Future rather than individual results seems to work well though.

@player-03
Copy link
Contributor

However, I discovered that since Promise is an @:generic class, it does not support static variables or functions.

Right, of course. I've always been unclear on why it was made generic in the first place.

@EliteMasterEric
Copy link
Contributor Author

Oh yeah I reworked to remove PromiseResult a while back but forgot to push it, here it is.

@player-03
Copy link
Contributor

I just merged #1846, so if you target 9.0.0-dev, you'll be able to add these directly to the class.

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