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

Make all() handle multiple rejected promises. #31

Closed
wants to merge 1 commit into from
Closed

Make all() handle multiple rejected promises. #31

wants to merge 1 commit into from

Conversation

calmh
Copy link
Contributor

@calmh calmh commented Mar 26, 2012

This fixes issue #30.

@rap1ds
Copy link
Contributor

rap1ds commented Apr 23, 2012

Would it make sense to call deferred.reject immediately, if the promise will be rejected anyway sooner or later? Is there a reason why wait all promises to be resolved/rejected?

IMO it would make more sense to call deferred.reject immediately and add an if(!rejected) so that deferred.reject wouldn't be called twice.

@rap1ds
Copy link
Contributor

rap1ds commented Apr 23, 2012

Another thing: If the promise is rejected but no error message or error object is passed, the deferred is resolved because if(!rejected) is truthy.

Anyway, I'm really hoping to see these issues fixed and the pull request to be merged. +1 These issues are kind of blockers to me.

@calmh
Copy link
Contributor Author

calmh commented Apr 23, 2012

@rap1ds I wanted to preserve the semantics of "all", i.e. wait for all promises to resolve (or reject) before continuing. Anyway, you're right about the second issue, a separate state variable is needed to handle the case of reject without an error object. I'm not really using promised-io any more but if you want to make a new pull request with the improved behavior, do so and I'll close this one instead.

@rap1ds
Copy link
Contributor

rap1ds commented Apr 23, 2012

@calmh Ok, I made a pull request. I decided to reject the all() as soon as possible because the original implementation did so too.

However, I can understand that in some cases it would make sense to wait for all promises before continuing.

@calmh
Copy link
Contributor Author

calmh commented Apr 27, 2012

Superseded.

@calmh calmh closed this Apr 27, 2012
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.

2 participants