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

Remove race() #81

Closed
wants to merge 3 commits into from
Closed

Remove race() #81

wants to merge 3 commits into from

Conversation

jsor
Copy link
Member

@jsor jsor commented Jan 8, 2017

See #57 for the discussion.

Closes #57

@jsor jsor added this to the v3.0 milestone Jan 8, 2017
@jsor jsor requested review from WyriHaximus and clue January 8, 2017 11:14
@clue
Copy link
Member

clue commented Jan 8, 2017

Let me quote from the linked issue:

Giving I have no current use for race(), I'm undecided.
Does removing it altogether really resolve the issue here? :)
My main concern here would be lack of documentation, but I'm good with either solution 👍

I'm currently undecided, but I'm okay with either.

@jsor
Copy link
Member Author

jsor commented Jan 9, 2017

A little bit more context: Our current implementation is not compatible with the ES6 specification because it does not return a forever pending promise if an empty array is given (currently, it returns a resolved promises with no value).

The discussion in #57 has been whether to remove it completely or make it ES6 compliant.

Given that race() has not been proven useful and even bluebird (apart from the native Promises, the defacto standard implementation of promises in JS) recommends against using it (http://bluebirdjs.com/docs/api/promise.race.html), this PR proposes to remove it.

@clue
Copy link
Member

clue commented Jan 9, 2017

The discussion in #57 has been whether to remove it completely or make it ES6 compliant.

IMHO consistency with ES6 makes sense, but it's not really a necessity because interoperability doesn't really apply across language boundaries anyway. Hence my vote to first document the current behavior and how this may differ from other implementations (afaict there's no mention of ES6 promises in the docs in the first place?).

@jsor
Copy link
Member Author

jsor commented Jan 9, 2017

@clue Consistency with ES6 in this case does not mean being consistent just for the sake of it but just to behave correctly.

Our documentation states:

Returns a promise which is resolved in the same way the first settled promise resolves.

So, the current implementation simply does not behave how we document it. It is arguable whether the return value should be a forever pending promise or if we should return an error (like we do with any() which rejects with a LengthException when the array is empty). But it should definitely not return a resolved promise which resolves with null.

@jsor
Copy link
Member Author

jsor commented Jan 11, 2017

I've opened an alternative PR which changes the behavior to return a forever pending promise: #83

@jsor
Copy link
Member Author

jsor commented Jan 14, 2017

Closed by #83

@jsor jsor closed this Jan 14, 2017
@jsor jsor deleted the remove-race branch January 14, 2017 12:28
@clue clue removed this from the v3.0.0 milestone Aug 31, 2022
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