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

fix(single): predicate function receives indices starting at 0 #2396

Merged
merged 1 commit into from
Feb 20, 2017

Conversation

martinsik
Copy link
Contributor

@martinsik martinsik commented Feb 20, 2017

When using the predicate function in the single() operator it passes indices starting at 1 instead of 0.

Observable.from(['a', 'b', 'c', 'd', 'e'])
  .single((val, i) => {
    console.log(val + ' ' + i);
    return val === 'b';
  })
  .subscribe();

This prints to console:

a 1
b 2
c 3
d 4
e 5

Live demo: https://jsbin.com/raveji/2/edit?js,console

That's because it's first incremented (single.ts#L65) and then read again from property (single.ts#L75).

In RxJS 4 the indices start at 0 (which is probably more expected behavior). Live demo: https://jsbin.com/qihaqur/3/edit?js,console

If the user is relying on indices starting at 1 than this could be a BC. Otherwise now it's compatible with RxJS 4.

There's one more inconsistency with RxJS 4.

When there's no item matching the predicate function the RxJS 4 version sends an error notification EmptyError. Live demo: https://jsbin.com/pukikes/3/edit?js,console

Observable.range(1, 5)
  .single(val => {
    return false;
  })
  .subscribe(
    val => console.log(val),
    err => console.log('error', err)
  );

This prints to console:

error { [EmptyError: Sequence contains no elements.] message: 'Sequence contains no elements.' }

But the RxJS 5 implementation just emits undefined as next and sends error only when the single() operator received no value at all (single.ts#L87).

Live demo: https://jsbin.com/yavepan/2/edit?js,console.

The docblock for single() says:

If the source Observable emits more than one such item or no such items, notify of an IllegalArgumentException or NoSuchElementException respectively.

This describes the RxJS 4 behavior but the RxJS 5 tests cover a use-case where single() emits undefined single-spec.ts#L142. Also the two exception classes mentioned don't exist.

So I'm wondering what's the correct behavior?

If the user is relying on indicies starting at 1 than this could be a BC. Otherwise now it's

compatible with RxJS 4.
@coveralls
Copy link

coveralls commented Feb 20, 2017

Coverage Status

Coverage decreased (-0.0007%) to 97.687% when pulling b29b8e3 on martinsik:fix-single into b9f611a on ReactiveX:master.

@kwonoj
Copy link
Member

kwonoj commented Feb 20, 2017

For behavior of single, back then I was implementing unmatched one returned undefined (#322 (comment)) and discussed to update to throw Error instead. I assume I have specific path left without updating it then.

@benlesh benlesh merged commit c81882f into ReactiveX:master Feb 20, 2017
@martinsik
Copy link
Contributor Author

@kwonoj I read the discussion and if I understand it correctly it was agreed that it should throw an error instead of undefined but it was later forgotten.

@lock
Copy link

lock bot commented Jun 6, 2018

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked as resolved and limited conversation to collaborators Jun 6, 2018
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.

4 participants