Skip to content

Conversation

@jsor
Copy link
Member

@jsor jsor commented Jul 19, 2016

This reverts automatic cancellation of pending collection promises once the output promise resolves. This was introduced in 42d86b7 and was both unintended and backward incompatible.

Note: This is for 2.x

CHANGELOG.md Outdated
* 2.4.2 (xxxx-xx-xx)

* Revert automatic cancellation of pending collection promises once the
output promise resolves. This was introduced in 42d86b7 and was both
Copy link
Member

Choose a reason for hiding this comment

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

42d86b7 should probably be replaced with a version tag and/or PR as these are more accessible.

Copy link
Member Author

@jsor jsor Jul 20, 2016

Choose a reason for hiding this comment

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

This has been 1 commit from the #36 PR, so i decided to use the commit. Will add the PR and version 👍

@clue
Copy link
Member

clue commented Jul 19, 2016

BC breaks is certainly something we want to avoid and it probably makes sense to revert this 👍

I'm unsure if the behavior the original change did introduce is not something others could possibly depend on, thus making this a BC break again :)

Does it make sense to preserve both behaviors through a toggle as originally discussed?

@jsor
Copy link
Member Author

jsor commented Jul 20, 2016

Good point, i'd like to avoid adding some toggle parameters to the function signatures though.

I'd propose adding notes both to the changelog and the readme to explain the behavior change and how to keep the automatic cancellation. What do you think?

@clue
Copy link
Member

clue commented Jul 21, 2016

Good point, i'd like to avoid adding some toggle parameters to the function signatures though.

I understand this, but I'd like to hear some more thoughts on this. Personally, I've used a very similar cancellation behavior in https://github.com/clue/php-block-react#awaitall. While this may be handy at times, this is not something that must be implemented in this promise library.

My position would be: I feel that it may make sense to keep both here, do we have any good reasons against this?

The changelog looks good to me (though the release tag may still be subject to discussion) 👍

According to semver, reverting a BC break should go into a new minor version.
@jsor
Copy link
Member Author

jsor commented Jul 21, 2016

My position would be: I feel that it may make sense to keep both here, do we have any good reasons against this?

I'd rather add a cancelAll helper function (like in your php-block-react) than a parameter. Thoughts?

The changelog looks good to me (though the release tag may still be subject to discussion)

Yes, thanks for the hint. Fixed the version. 👍

@jsor jsor added this to the v2.5 milestone Jul 21, 2016
@jsor
Copy link
Member Author

jsor commented Oct 24, 2016

I'd like to make progress on this. Any thoughts on this? Ping @cboden @WyriHaximus @clue

Copy link
Member

@WyriHaximus WyriHaximus left a comment

Choose a reason for hiding this comment

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

LGTM :shipit:

@jsor jsor merged commit 9547056 into 2.x Nov 9, 2016
@jsor jsor deleted the revert-cancellation-of-pending-collection-promises branch November 9, 2016 19:52
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