Feature Request: Map() or Extend function to FirebaseListObservable() for list items

[DennisSmolek]

Hey guys, I'm doing a few things with FirebaseListObservables that I'd like to be able to extend or map other things onto the items themselves but can't.

consider:

let obj = this.af.database.object('/numberseven'); // the number 7
let multi = obj.map(inner => inner * 10);
multi.subscribe(multiplied => console.log('Should be 70:', multiplied)); // console says 70

You can't do this to the list object as it always returns an Array so you have to double wrap it:

let obj = this.af.database.list('/numbers'); // [5, 6, 7, 8];
let multi = obj.map(innerArray => innerArray.map(num => num * 10));
multi.subscribe(multiplied => console.log('Should be list of numbers:', multiplied)); // [50, 60, 70, 80]

The problem is that it has to run for EVERY item in the array and with more complex mapped functions this becomes a huge performance suck.

What would be cool is a way to do it like:

let mapped = this.af.database.list('/numbers').map((num) => num * 10);
mapped.subscribe(mappedArray => console.log('mapped array:', mappedArray)); // [50, 60, 70, 80]

That only runs once as the item is actually changed/updated/whatever.

Now, I haven't spent a lot of time on this, but I think something like this would work:

function mapFn(num) { return num * 10};
let mapped = this.af.database.list('/numbers', {map: mapFn});

Then modify either the utils.unwrapMapFn in firebase_list_factory or just add it to the FirebaseListFactoryOps class and modify the toValue()

something like:

const toValue = preserveSnapshot ? (snapshot => snapshot) : (mapFn ? mapFn((snapshot => utils.unwrapMapFn(snapshot))) ? utils.unwrapMapFn);

This way we can apply functions directly to the children as they get added to the inner array only once.

There are a million ways to do this I know, but I think it would be helpful in the main codebase somewhere...

[davideast]

@DennisSmolek I'm definitely interested! Are you up for doing some research with an experimental fork?

[DennisSmolek]

Sure!
I've got a few things this week taking my attention but I can start messing with it. Besides simply getting it to work and setting up the tests what else do you recommend I test/experiment for?

[davideast]

I'd love to see if there's a way to measure the performance impact it has, so any metrics would be nice.

[cartant]

@DennisSmolek I've created an issue (#883) for a proposal to address a problem that I have that's closely related to yours. Please check it out to make sure that I've understood what you've posted here and that the proposal will solve your issue as well as mine.

[DennisSmolek]

@cartant I read over your issue, I haven't fully digested it but the point about performance I was making was in regards to digestion and subsequent change detection.

Internally, AngularFire uses an array to store the results of items returned from various standard firebase change detections (on, once). When these detections cause changes the entire array set is returned as the value of the array and not as a reference so the object must run any subsequent actions to the payload against each item in the array.

this means if I have a heavy mapped function on a list of items, say to attach user data to a chat post, that request runs over data it's already done so to, as well as triggering basic change detection. By extending the ability to have a map function before adding to it's internal array it would let me make my own adjustments.

I haven't had time to add this to the library yet as it's nuts here but something like this:

function handleSnapMap(snap, mapFn?) {
    let item = snap.val();
    return mapFn ? mapFn(item) : item;
}
function makeListObs(path: string, mapFn?: any) {
return Observable.create(observer => {
        let collection: Array<any> = [];
        let ref = firebase.database().ref('/items');
        // set array
        ref.once('value').then(firstSnapshot => {
            firstSnapshot.forEach(childSnap => {
                collection.push(handleSnapMap(childSnap));
            });
            //fires once collection has loaded
            observer.onNext(collection);
        });
        // listen for new, allow extension
        ref.on('child_added', (singleSnapshot) => {
            collection.push(handleSnapMap(singleSnapshot.val()));
            observer.onNext(collection);
        });

    }); // obs
}// n

[cartant]

@DennisSmolek Yeah, the expensive DOM updates for unchanged items are something I really want to avoid, too.

PR #792 introduced a change to ensure that unwrapMapFn is called only when items are actually changed, so the proposal would give you something that you could use for your own mapping as well as unwrapping. And it would only be run when items are first received or changed.

So, using your example, you'd do something like this:

this.af.database.list('/numbers', {
  unwrapSnapshot: (snapshot) => snapshot.val() * 10
});

The unwrapping and the mapping would be done in the one function. And if you need the original unwrap implementation, you could import unwrapSnapshot (renamed from unwrapMapFn) and call that in your own function.

[somombo]

@cartant @DennisSmolek,

I believe that what we are trying to implement here and in #888 / #883 is referred to throughout rxjs as a "selector". See several examples on the official rxjs observable API reference page.

Here is an example of how I have been using selectors with underlying firebase api, to get around our problem.

// ...
const childAdded = Observable.fromEventPattern(
  (handler:QueryHandler) => { messagesQuery.on("child_added", handler) },
  (handler:QueryHandler) => { messagesQuery.off("child_added", handler ) },
  (snapshot:Snapshot): Message => unwrapSnapIntoMsg(snapshot) // <-- selector parameter
)
// ...

... I do this for child_changed and child_removed as well and then merge the three observables into a single stream.

As is shown in rxjs docs, fromEventPattern's third parameter is a selector in precisely the kind of way that we are looking to implement.

As such, I recommend that when we do implement this feature, we call it "selector" for consistency. From rxjs docs, it seems to almost be ubiquitously understood what selector means. I think that will make it somewhat easier to document the feature and lead to less confusion.

I also think that for consistency with rxjs api style, the selector should be the third parameter of FirebaseListFactory rather than lumped up as another FirebaseListFactoryOpts option. This would look like:

this.af.database.list('/numbers', 
  { preserveSnapshot: true, query: {} }, // <-- or `undefined`
  (snapshot) => snapshot.val() * 10  // <-- third parameter in style of rxjs
);

Thoughts?

[cartant]

@somombo I can see what you mean, but my reservation is that selectors are usually passed the value that would otherwise be emitted by the observable. It seems a strange to have a selector that receives a snapshot when the observable would otherwise be emitting an unwrapped snapshot - for a FirebaseObjectObservable - or an array of unwrapped snapshots - for a FirebaseListObservable. Do you have any examples of selectors in the RxJS API that receive something different to what would otherwise be emitted by the observable? I'm open to suggestions; my primary concern is that the efficiency issue is resolved, as at the moment, it pretty much breaks OnPush change detection.

[somombo]

@cartant Thanks for the timely feedback,

I cant seem to think of an example of a selector that does not receive a value that would otherwise be emitted (e.g. like in our case, the selector does not receive the current list array even though that's what's ultimately emitted ... perhaps we may add this as an extra feature of our selector in case there are people that might use it? For now, not critical though).

However, what I do have, is an example of a selector that receives a value that is otherwise not emitted (e.g. in our case, the selector receives wrapped/unwrapped snapshot yet the snapshot is merely a list-item of the Array that will ultimately emitted).

Example of the latter, is resultSelector of switchMap whose interface is as follows:

resultSelector: function(outerValue: T, innerValue: I, outerIndex: number, innerIndex: number): any

The innerValue parameter , for example, is not otherwise emitted, and one presumable could do as she pleases with it, within the body of the selector (including, i imagine, side effect stuff).

There are many different types of selectors mentioned in the rxjs API e.g. durationSelector, keySelector, elementSelector. Since ours in its current form applies to list items and not to the list itself, I suppose we could call it an listItemSelector or itemSelector or snapshotSelector. If we intoduce the option for the entire list to be past on to the the selector (e.g. if the output snaption will somehow depend on the current state of the entire list), then I'd lean towards simply calling it a selector.

Lastly, I do hear your concerns about how this issue currently affects change detection n' effeciency. I think this is a primary issue for me as well. I just wanted to make sure that we dont make it harder to streamline our public API towards consistency with rxjs in future, because of fear of breaking changes.

[cartant]

@somombo Thanks for the examples. I agree that the unwrapSnapshot option would be better named as some type of selector, but I'm not convinced that it should be a separate parameter. The list and object methods are already a little different to the RxJS API as they take an options parameter. Having some options in options and some outside seems a little weird.

Anyway, I'll wait until there is some action on the reviewing of the PR before considering or suggesting a rename, etc. Also, you might want to make any further comments against the PR itself, where they are more likely to be seen by a reviewer.

[HunderlineK]

I might be asking a question with an obvious answer, but what was the reason for introducing the FirebaseListObservable type in the first place? I might be just imagining things, but the last time I used firebase I had more methods available for the Observable type?

Reading @DennisSmolek comment, am I wrong to assume that FirebaseListObservable uses the firebase once method internally and might not really be a pure Observable?

[DennisSmolek]

@HoumanKml it is an observable but because you can no longer loop over Objects in Angular they have the FirebaseListObservable so the resulting item is an array as well as convenience methods like forEach baked in. It also puts the key on each item in the array automatically.

[HunderlineK]

@DennisSmolek That is interesting, I wasn't aware of that! It is just that it seems I can't use typical rxjx operators on the firebaseListObservable? (which is weird, because I just checked and firebaseListObservable extends Observable). I think I am just confused about the way one is expected to work with angularfire.

[bjunix]

@HunderlineK FirebaseListObservable indeed extends Observable but you need to import the operators manually to have them available, like this:

import 'rxjs/add/operator/map';

This is a bit buried in the Angular docs. I could only find it here:
https://angular.io/tutorial/toh-pt6#import-rxjs-operators

[davideast]

@DennisSmolek Would the new API with .stateChanges() suit your needs?

[somombo]

@davideast could you show an example snip of code that exemplifies how to achieve what was requested in this feature using the .stateChanges()?

[DennisSmolek]

@davideast I think it will.. I'm still mulling over how I feel about the loss of $key but I think this issue specifically is resolved with .stateChanges() as the examples all use map() for the key value.