Skip to content

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

Closed
byrondover wants to merge 1 commit into
angular:masterfrom
byrondover:bugfix/async-alternative
Closed

byrondover wants to merge 1 commit into
angular:masterfrom
byrondover:bugfix/async-alternative

Conversation

@byrondover

Copy link
Copy Markdown
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
Copy Markdown
Contributor

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
Copy Markdown

@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