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

FirebaseListObservables always emit different element instances for unwrapped snapshots #791

Closed
cartant opened this issue Jan 26, 2017 · 1 comment · Fixed by #826
Closed

Comments

@cartant
Copy link
Contributor

cartant commented Jan 26, 2017

The reducers that manage a FirebaseListObservable's internal array call the unwrapMapFn utility function when emitting (see here, here, here and here).

This is less that ideal, as it means that (when not preserving snapshots) the elements in the emitted arrays will always be different object instances between emissions. That does not play nice with Angular's OnPush change detection - which takes advantage of unchanged object references - and always emitting different objects incurs a significant performance penalty.

Version info

Angular: 2.4.5

Firebase: 3.4.0 (for Plunker compatibility)

AngularFire: 2.0.0-beta.7

How to reproduce these conditions

There is a plunk here.

Steps to set up and reproduce

See plunk above. To see the difference in performance:

  • run the plunk
  • wait for the lists to start rolling (they should roll alternately, every second)
  • open up the dev tools and record a profile
  • wait for a few seconds, allowing each list to roll several times
  • stop the profiling
  • look at the flame graph, noting the performance difference between the preserved (fast) and unwrapped (slow) change detection and DOM updates

Expected behavior

The arrays emitted by lists that unwrap snapshots should use the same unwrapped object for array elements that have not changed.

Actual behavior

Each time arrays of unwrapped snapshots are emitted by lists, the array elements are always different object instances - even if the elements have not actually changed.

cartant added a commit to cartant/angularfire that referenced this issue Jan 26, 2017
Store either unwrapped snapshots or preserved snapshots; don't store
preserved snapshots and call utils.unwrapMapFn when emitting.

Closes angular#791
cartant added a commit to cartant/angularfire that referenced this issue Jan 26, 2017
Store either unwrapped snapshots or preserved snapshots; don't store
preserved snapshots and call utils.unwrapMapFn when emitting.

Closes angular#791
@cartant cartant changed the title FirebaseListObservables always emit different element instances for non-preserved snapshots FirebaseListObservables always emit different element instances for unwrapped snapshots Jan 26, 2017
cartant added a commit to cartant/angularfire that referenced this issue Jan 26, 2017
Store either unwrapped snapshots or preserved snapshots; don't store
preserved snapshots and call utils.unwrapMapFn when emitting.

Closes angular#791
@davideast
Copy link
Member

Amazing work per usual @cartant! Thanks for finding this, bugging it, and then sending in a PR! We are lucky to have you as a contributor.

davideast pushed a commit that referenced this issue Jan 30, 2017
Store either unwrapped snapshots or preserved snapshots; don't store
preserved snapshots and call utils.unwrapMapFn when emitting.

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

Successfully merging a pull request may close this issue.

2 participants