search

FirebaseExtended / rxfire

Apache License 2.0
139 stars 40 forks source link

filterEvents() implemented incorrectly #55

Open Timebutt opened 2 years ago

Timebutt commented 2 years ago

I ran into an obscure bug when using @angular/fire in my application and traced it down to an (in my opinion) incorrect implementation of filterEvents in rxfire (implementation here).

To my surprise, when using collectionChanges() in tandem with a query() that produces no results, I never get an emission from collectionChanges()! I'm expecting the stream to return an empty array [] when the query has completed without results, in line with how default firebase operates, as I need to know when the request completed (to stop rendering a spinner for instance, ...). Looking through code, I currently don't see any way of configuring the method in such a way that it would produce an empty array instead of never emitting.

The above behaviour stems from the fact that filterEvents() is implemented very differently from how I would assume it to work. From my perspective, that method should only pass elements that match the requested DocumentChangeType. What it does now is different however: if at least one item matching said filter is found, all elements from the array are passed, and if no element is found the emissions is blocked entirely.

Is this truly the intended behaviour? If it is, looks like I'll have to live with patching rxfire to make sure I can render loading states correctly. If this isn't intended behaviour, I'd be very happy to submit a PR to fix this! Curious to hear what your thoughts are.

Benny739 commented 2 years ago

same problem here

jamesdaniels commented 2 years ago

Hmmm, filterEmptyUnlessFirst should cover that case and emit an empty array. Perhaps the implementation in rxfire didn't capture this behavior correctly when it was ported from AngularFire compat or we regressed in modular.

Timebutt commented 2 years ago

So I just checked the implementation of filterEmptyUnlessFirst, but I fail to see how the implementation would produce an empty array when no value is emitted from filterEvents.

If filterEvents() does not emit a value (in case of no query results, or no changes matching the requested type), filterEmptyUnlessFirst() will first call windowwise() which has a startWith(undefined) but the pairwise() on the line below will never fire, as there never is a second emissions apart from the startWith(), effectively blocking an emission for the main collectionChanges() stream.

My main question is, if the filterEvents() is supposed to prevent emitting at all when no matching results could be found?