Add type and default generic type to DataSnapshot

[iNViTiON]

Allow AngularFire2 list snapshotchanges to have generic type support.

This not affect any usage since it included default type.

[hiepxanh]

Hello, is that any problem with that changes? I think it would be nice for angularfire2. Hope it will be merge soon

@schmidt-sebastian

[schmidt-sebastian]

The RTDB SDK for Android handles this situation a little differently. We have getValue() overloads that allow you to specify the return type (https://firebase.google.com/docs/reference/android/com/google/firebase/database/DataSnapshot.html). This nicely works around the problem of adding type information to DataSnapshot itself, which should theoretically also result in changes to all API methods that return DataSnapshots(*).

@iNViTiON, @hiepxanh: Could we get away with something simply as:

val<T>(): T;

If that doesn't help you, then I am fine to merge this change as is.

(*) Please don't do this, as this doesn't quite work for a schema-less database.

[iNViTiON]

@schmidt-sebastian that will cause something like this to happen.

let stuff = this.db.list<number>('/stuff'); // stuff is a list of numbers, sure

let stuff$ = stuff.snapshotChanges<string>(); // wait, it's a list of strings now?

stuff.push(1); // But I can only push a number and not string?

[hiepxanh]

oh my god, I am about to call you @iNViTiON 😄 I thought David already have a commit and closed that issue?

[iNViTiON]

@hiepxanh that commit only for valueChanges but not affect snapshotChanges. My PR rely a lot on his PR. 😅

[schmidt-sebastian]

Please make the same change in

firebase-js-sdk/packages/firebase/index.d.ts

Line 427 in b351749

interface DataSnapshot {

and we are good to go.

This change here is fine if it addresses a need for AngularFire2. In general, adding typings to DataSnapshot enforces a schema when we we don't really have one (note that your stuff.push(1) example above is perfectly valid). Since we are leaving the default as "any", I am fine with merging this.

Please note that there are other places in the API where it might make sense to add generics to DataSnapshot (such as DataSnapshot.child and DataSnapshot.forEach). If this is a requirement, then I will likely push back on this later and ask to go more towards what we have on Android.

[hiepxanh]

yahoo! thank you for willing accept to that PR. 😍

[iNViTiON]

@schmidt-sebastian pushed and waiting for CI result. I also added nullable to val() in this commit since it will return null when the path was empty. Is this a right thing to do?

[jshcrowthe]

LGTM 👍 leave it up to you @schmidt-sebastian

[wilhuff]

How does this change help anything?

const ref = db.collection('foo').doc('bar');
const snap = await ref.get();

In this snippet snap has type DocumentSnapshot<any>. If you want it to be some other parameter you're going to have to cast. If you have to cast anyway, why not cast the result of val()? I.e. why is

const snap = await ref.get() as DocumentSnapshot<T>;
const val = snap.val();

better than

const snap = await ref.get();
const val = snap.val() as T;

?

[iNViTiON]

@wilhuff Because AngularFire2 have enforce schema since AngularFireObject or AngularFireList was create. If we cast value alone it can cause inconsistent data type like this.

[jamesdaniels]

FYI I've done some interface work for AngularFire in angular/angularfire#1644 and angular/angularfire#1643; seems like there's some duplicate work going on here.

I'm happy with my changes which extend DataSnapshot, DocumentSnapshot, and QueryDocumentSnapshot living in AngularFire but maybe we want to consider bringing it in here?

Also making a union of exist vs. not cases allows for the use of type guards so one doesn't have to use bangs on val, which is nice.

[schmidt-sebastian]

@iNViTiON Sorry to keep this unmerged for so long. We would like to address how to handle the typings for AngularFire for both the Realtime Database and Firestore before we go ahead and merge this. I will update you (and hopefully merge this PR) by end of week.

[iNViTiON]

Thanks, @schmidt-sebastian. @jamesdaniels maybe we should wait until @firebase team settled this. So @AngularFire2 code can fully support it in the same direction.

[jamesdaniels]

@iNViTiON I think @davideast and I are comfortable having extended types in AngularFire for now; we already have quite a few. In fact, we just merged angular/angularfire#1643, it will be included in the next @next release. We are on the Firebase team BTW, I sit next to @schmidt-sebastian in the office, so it's not like we're going too far out on a limb here ;) I'd be happy if our typing enhancements did make it into core though; as it would mean less things we have to maintain, less chance of breakage, and more typescript goodness for the TS community that may not be using AngularFire.

@schmidt-sebastian I'm WFH and trying my best to avoid meetings today, but let's find some time this week to discuss.

[iNViTiON]

@jamesdaniels yes, I notice you're in Firebase team but I'm not sure how the team manage this. That a good news. 😊
I'm not complain about extended type since I'm not the one to maintain it but just hope to decrease your work. The fast it comes out the good thing for me too. Hope it came out well.
P.s. I'm not English native speaker. Please don't mind me if I choose wrong word or some bad grammars. No bad intention.

[schmidt-sebastian]

Quick update: We discussed this PR today and are investigating if we can push this change down to AngularFire in its entirety.

[schmidt-sebastian]

As per @jamesdaniels, this issue is now fulled addressed in AngularFire2 (per angular/angularfire#1643). We might revisit pushing some of this typing down to the main JS client, but will hold off on it for now.