ReactiveX / rxjs

A reactive programming library for JavaScript
https://rxjs.dev
Apache License 2.0
30.7k stars 3k forks source link

find yields single value (undefined) when no match #2515

Closed gluck closed 7 years ago

gluck commented 7 years ago

RxJS version:

5.2.0 / master

Code to reproduce:

Observable.empty().find(_ => false)

Expected behavior:

Should not yield any result, as expected (and as per rxjs4 behavior and doc), cf unit test: https://github.com/Reactive-Extensions/RxJS/blob/master/tests/observable/find.js#L63

Actual behavior:

Yields a single value (undefined) before completion, as per: https://github.com/ReactiveX/rxjs/blob/master/src/operator/find.ts#L104

Additional information:

I believe the cause is that both find/findIndex operators were implemented by same subscriber, and findIndex does emit a value (-1) when not found (also in rxjs4).

gluck commented 7 years ago

(can work out a PR if you confirm rxjs 4 is the wanted behavior)

david-driscoll commented 7 years ago

The behavior makes sense to me. cc @kwonoj @mattpodwysocki @benlesh

kwonoj commented 7 years ago

Yes, I think it's my fault when I implemented this (https://github.com/ReactiveX/rxjs/pull/393) while Rx v4 doc states (https://github.com/Reactive-Extensions/RxJS/blob/master/doc/api/core/operators/find.md)

An Observable sequence with the first element that matches the conditions defined by the specified predicate, if found; otherwise, an empty sequence.

case of find in opposite to findIndex.

felixfbecker commented 7 years ago

The current signature also is misleading, it says to return Observable<T> when it really returns Observable<T | undefined>. Met this bug through a "Cannot read x of undefined" Error today. If #2538 is not merged until next major, the signature should be fixed.

david-driscoll commented 7 years ago

We can't adjust the interface until the next major anyway because we're not yet using a version of TypeScript that supports the unit types null or undefined.

kwonoj commented 7 years ago

Closing as changes are checked in.

lock[bot] commented 6 years ago

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.