Skip to content

Proposal for alternative snapshot unwrapping implementations #883

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
cartant opened this issue Apr 2, 2017 · 2 comments
Closed

Proposal for alternative snapshot unwrapping implementations #883

cartant opened this issue Apr 2, 2017 · 2 comments

Comments

@cartant
Copy link
Contributor

cartant commented Apr 2, 2017

With the merging of PR #787 (for issue #302), I have a problem. The $key added to unwrapped snapshots is now not included in serialized items. This is an issue for me as it breaks the logging implementation that I use and also results in items that I do not want to put into a Redux store (@ngrx/store) as they are not serializable (well, not without losing the $key).

It's simple enough to use a map operator to change the items, but that introduces another problem: if a map is used on each item, the performance is massively downgraded as OnPush change detection will see each item in the emitted list as being a different instance - even if item has not actually changed. This essentially the same issue that was fixed in PR #792 and the plunk in issue #791 demonstrates the performance impact.

This is also closely related to issue #848. That issue mentions the performance impact of having to apply a secondary map. That issue's author mentions the performance penalty, but it's not clear whether the author is talking about the computational cost or the DOM manipulation cost, but the latter is likely to be the bottleneck.

To solve the above problems, the proposal is to:

  • move database-specific interfaces into database/interfaces.ts
  • move database-specific utils into database/utils.ts
  • rename unwrapMapFn to unwrapSnapshot
  • move unwrapSnapshot into database/unwrap_snapshot(.spec).ts, etc.
  • expose unwrapSnapshot as an export
  • add an UnwrapSnapshotProvider that provides the default/current implementation, etc.
  • add an unwrapSnapshot option to FirebaseListObservable, etc.

It will be a non-breaking change that will allow for the wholesale injection of an alternative unwrap implementation and for the case-by-case specification of per-list unwrap implementations.

@cartant
Copy link
Contributor Author

cartant commented Apr 3, 2017

Here are some contrived examples to demonstrate the proposal's usage.

A snapshot could be mapped to a single property by doing something like this (this is not a particularly compelling use case, but it serves as an example):

const names = this.af.list('things', {
  unwrapSnapshot: (snapshot) => snapshot.val().name
}) as Observable<string>;

A list observable's items could be mapped to instances of a particular class by doing something like this:

class Thing {
  constructor(snapshot: firebase.database.DataSnapshot) {
    ...
  }
}

const things = this.af.list('things', {
  unwrapSnapshot: (snapshot) => new Thing(snapshot)
}) as Observable<Thing>;

A $save method could be added to a list observable's items by doing something like this:

import { unwrapSnapshot } from 'angularfire2/database';

const things = this.af.list('things', {
  unwrapSnapshot: (snapshot) => {
    const unwrapped = unwrapSnapshot(snapshot);
    Object.defineProperty(unwrapped, '$save', {
      value: () => snapshot.ref.set(unwrapped),
      enumerable: false
    });
    return unwrapped;
  }
});

The wholesale replacement of the unwrapSnapshot implementation could be acheived by injecting an alternate implementation, like this:

import { UnwrapSnapshotToken } from 'angularfire2/database';

function unwrapSnapshotToSerializable(
  snapshot: firebase.database.DataSnapshot
): any {
  ...
}

@NgModule({
  ...
  providers: [
    {
      provider: UnwrapSnapshotToken,
      useValue: unwrapSnapshotToSerializable
    },
    ...
  ]
}

cartant added a commit to cartant/angularfire that referenced this issue Apr 6, 2017
And add a provider for the default unwrapSnapshot implementation.

Closes angular#883 and angular#848.
cartant added a commit to cartant/angularfire that referenced this issue Apr 6, 2017
And add a provider for the default unwrapSnapshot implementation.

Closes angular#883 and angular#848.
cartant added a commit to cartant/angularfire that referenced this issue Apr 6, 2017
And add a provider for the default unwrapSnapshot implementation.

Closes angular#883 and angular#848.
cartant added a commit to cartant/angularfire that referenced this issue Apr 6, 2017
And add a provider for the default unwrapSnapshot implementation.

Closes angular#883 and angular#848.
@somombo
Copy link
Contributor

somombo commented Apr 15, 2017

I like the spirit of this. Look forward to seeing your pr or something similar merged soon.

@cartant cartant closed this as completed May 10, 2017
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

No branches or pull requests

2 participants