Skip to content

fix(db): coerce async snapshot subscriptions with debounce operator #1588

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

Closed

Conversation

byrondover
Copy link
Contributor

Checklist

Description

Swaps delay(0) in favor of debounceTime(0) to coerce async fromRef() database snapshot subscriptions. This prevents async listChanges() scans from burst emitting multiple copies of the same ref['on']('value') events.

@jamesdaniels Is this an acceptable / desirable change? It works well for me, but may have implications I'm not thinking of outside of standard RTDB use cases.

An alternative might be to revisit how listChanges() is implemented, perhaps by augmenting distinctUntilChanged() to debounce objects / arrays as well as immutable primitives.

Code sample

src/database/observable/fromRef.ts

export function fromRef(ref: DatabaseQuery, event: ListenEvent, listenType = 'on'): Observable<AngularFireAction<DatabaseSnapshot>> {
   ...
   // Ensures subscribe on observable is async. This handles
   // a quirk in the SDK where on/once callbacks can happen
   // synchronously.
-  .delay(0)
+  .debounceTime(0)
   .share();
 }

@byrondover byrondover changed the title Coerce async database snapshot subscriptions with debounce operator fix(db): coerce async snapshot subscriptions with debounce operator May 3, 2018
@jamesdaniels
Copy link
Member

I have a suspicion on both this and the root cause of your slow down now; I'm going to close this while I think on it more + come up with an ultimate fix.

I think the .share got moved to a different place, leading to multiple listeners being created w/your usage patterns. While it's not changing the data it's leading to unnecessary change detection since we're slicing the array. distinctUntilChanged() is thinking in the right direction, but I think ultimately the correct thing to do is be more careful about using slice and creating a new array (which trips change detection, since === is false) when the snapshotChange is a no-op.

@tayambamwanza
Copy link

@byrondover That change fixed everything for me, I'll use it until another official fix is developed. Thank you so much you saved me a lot of hours 🤣 💐 !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants