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

feat(operator): add single operator #322

Closed
wants to merge 1 commit into from

Conversation

kwonoj
Copy link
Member

@kwonoj kwonoj commented Sep 16, 2015

Continue effort to add operator exists in current RxJS.

@benlesh
Copy link
Member

benlesh commented Sep 17, 2015

Actually, now that I'm digging into this operator I rarely use and researching the C# semantic it was born from, it looks like current RxJS and this impl are doing something weird with it.

Enumerable.Single() asserts that there is a single element in the enumerable.

likewise Observable.Single() does the same.

The current RxJS version is like an alternate version of first(), to me.

We could actually benefit from a single() that asserted a single item in an Observable, because it could return a ScalarObservable, which has optimizations in parts of our code. Also it would provide functionality that first() did not.

@mattpodwysocki, what are your thoughts?

@kwonoj
Copy link
Member Author

kwonoj commented Sep 17, 2015

Personally I agree with @Blesh. As already mentioned currently single does nearly same as first.

@mattpodwysocki
Copy link
Collaborator

@kwonoj disagree completely here. We are ensuring with single there is one and ONLY one of those that match in the collection which can be very useful. First does not cover that. According to @jhusain he says he uses single quite a bit versus first

@benlesh
Copy link
Member

benlesh commented Sep 17, 2015

@kwonoj @mattpodwysocki ... oh... I think I've conflated some things here.

The behavior of single in our library should say the same as it is in RxJS 2/3/4... which is precisely what I linked above. I thought it was doing something different.

So:

Observable.of(1).single(); // 1
Observable.of(1,2).single(); // error more than one element
Observable.of(1,2).single(x => x > 1); // 2
Observable.of(1,2,3).single(x => x > 1); // error more than one element
Observable.of(1,2,3,1).single(x => x === 1); // error more than one element

I'll have to look over the tests again to see if this is actually the case.

@benlesh
Copy link
Member

benlesh commented Sep 17, 2015

In other words, I think we all agree and I just said something confusing above. :p

@kwonoj
Copy link
Member Author

kwonoj commented Sep 17, 2015

@mattpodwysocki Thanks for pointing out and clarification.

@kwonoj
Copy link
Member Author

kwonoj commented Sep 17, 2015

Updated behavior as well as test specs, should be same as existing RxJS does.

  • ensure only single matching element exists from given predicate. if predicate is empty, ensure whole observable contains only single element.
  • raise error if multiple matching element found
  • returns undefined if there isn't any matching element

it('Should return undefined from predicate if observable does not emit', function() {
var e1 = hot('--a--^--|');
var expected = '---(x|)';

Copy link
Member Author

Choose a reason for hiding this comment

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

Previous comment seems hidden by updated commit, leaving note again for reference.

This test case illustrates case like example in document
Rx.Observable.empty().single()

where source does not emit anything also predicate is empty to specified match any element. In this case current RxJS behaves same as cases in line 91 which source emits but there isn't any matching element by predicate, returns undefined.

Copy link
Member

Choose a reason for hiding this comment

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

That seems broken. @mattpodwysocki why does this not error that the sequence was empty?

@benlesh
Copy link
Member

benlesh commented Sep 18, 2015

@kwonoj checking in... what's the status of this PR? are you ready for me to pull this down and test it? Do you need assistance?

Thanks again for your hard work.

@kwonoj
Copy link
Member Author

kwonoj commented Sep 18, 2015

This PR have one remaining question to be clarified why Obervable.empty().single() returns undefined in existing RxJS which this PR mimics it. (maybe waiting @mattpodwysocki ?)

Other than those, I believe rest of changes are ready.

@benlesh
Copy link
Member

benlesh commented Sep 18, 2015

After having discussed this with @benjchristensen, @jhusain and @trxcllnt, I think the concensus is that is should error.

This is because single() asserts in all other cases, and because you'd have no way to tell the difference between Observable.of(undefined).single() and Observable.empty().single().

If the desired behavior for the user is to emit undefined from an empty Observable with single() they should do the following:

someObservable.single().catch(err => err === Rx.EmptyError ? Observable.of(undefined) : Observable.throw(err))

TL;DR: have it throw an EmptyError (which already exists in the lib)

@kwonoj
Copy link
Member Author

kwonoj commented Sep 18, 2015

Make sense to me. I'll update PR soon with changes. Appreciate for clarification!

@benlesh
Copy link
Member

benlesh commented Sep 18, 2015

We can then merge and close this PR. If @mattpodwysocki or @headinthebox provides the reasoning behind the nexting of undefined from the original libraries, we can always change it back. But everyone I discussed this with didn't know why it does what it does.

@kwonoj
Copy link
Member Author

kwonoj commented Sep 18, 2015

Updated behavior as well as test case to raise error if source observable does not emit.

@benlesh
Copy link
Member

benlesh commented Sep 18, 2015

Merged as of 49484a2

Thanks for all of your hard work, @kwonoj! I'm really happy with the discussion and clarification around this issue/PR in general. Thanks to everyone for helping resolve the Observable.empty().single() behavior.

@benlesh benlesh closed this Sep 18, 2015
@kwonoj kwonoj deleted the feat-operator-single branch September 18, 2015 21:34
@lock lock bot locked as resolved and limited conversation to collaborators Jan 19, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants