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

Use Promise.allSettled instead of Promise.all to finish execution of all listeners #285

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

Conversation

onhate
Copy link

@onhate onhate commented Mar 2, 2022

closes #283

On an application with multiple listeners to the same event when using Promise.all It rejects immediately upon any of the input promises rejecting or non-promises throwing an error, and will reject with this first rejection message / error.

While Promise.allSettled is typically used when you have multiple asynchronous tasks that are not dependent on one another to complete successfully, or you'd always like to know the result of each promise, which is the use case of having multiple listeners for the same event.

@onhate
Copy link
Author

onhate commented Mar 4, 2022

@DigitalBrainJS what do you think?

P.S.: Just now I saw you live in Kyiv, hope you and your family are safe

@RangerMauve
Copy link
Contributor

Thank you for your patience, I've been AFK from open source projects due to having way too much to do with client work.

Would you mind running benchmarks before and after so we could compare performance implications before we merge this in?

@j0tunn
Copy link

j0tunn commented Jan 13, 2023

@onhate Hi! May be an option to enable Promise.allSettled behaviour would be a better solution.
I think those who want an allSettled behaviour wouldn't pay attention to possible performance issues.

@onhate
Copy link
Author

onhate commented Jan 13, 2023

Technically there are no differences on performance between all and allSettled, I couldn't find any article indicating that at least, they are both v8 implementations running on the core of it, I even looked on the source code of v8 and both implementations run the same way calling the function GeneratePromiseAll but with just an extra param that indicates to throw on first rejection or not.

https://github.com/v8/v8/blob/main/src/builtins/promise-all.tq#L376

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.

Why not Promise.allSettled instead of Promise.all?
3 participants