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

valueChanges does not inherit type of list/object #1214

Closed
Maistho opened this issue Oct 5, 2017 · 6 comments
Closed

valueChanges does not inherit type of list/object #1214

Maistho opened this issue Oct 5, 2017 · 6 comments

Comments

@Maistho
Copy link
Contributor

Maistho commented Oct 5, 2017

Version info

Angular: 4.4.4

Firebase: 4.5.0

AngularFire: 5.0.0-rc.0-next

The problem

AngularFireList and AngularFireObject has a generic T that is not inherited to all of it's members, such as valueChanges or snapshotChanges.

Is it possible to have a generic for snapshotChanges? It returns a SnapshotAction, which in turn contains a firebase.database.DataSnapshot, which is not generic.

Links to related Interfaces
AngularFireList
AngularFireObject

The fix

(if I'm correct)

--- a/src/database/interfaces.ts
+++ b/src/database/interfaces.ts
@@ -5,7 +5,7 @@ export type FirebaseOperation = string | firebase.database.Reference | firebase.
 
 export interface AngularFireList<T> {
   query: DatabaseQuery;
-  valueChanges<T>(events?: ChildEvent[]): Observable<T[]>;
+  valueChanges(events?: ChildEvent[]): Observable<T[]>;
   snapshotChanges(events?: ChildEvent[]): Observable<SnapshotAction[]>;
   stateChanges(events?: ChildEvent[]): Observable<SnapshotAction>;
   auditTrail(events?: ChildEvent[]): Observable<SnapshotAction[]>;
@@ -17,8 +17,8 @@ export interface AngularFireList<T> {
 
 export interface AngularFireObject<T> {
   query: DatabaseQuery;
-  valueChanges<T>(): Observable<T | null>;
-  snapshotChanges<T>(): Observable<SnapshotAction>;
+  valueChanges(): Observable<T | null>;
+  snapshotChanges(): Observable<SnapshotAction>;
   update(data: T): Promise<any>;
   set(data: T): Promise<void>;
   remove(): Promise<any>;

Should I open a PR?

@Toxicable
Copy link

valueChanges does return an Observable<T[]>
https://github.com/angular/angularfire2/blob/master/src/database/interfaces.ts#L8

@Maistho
Copy link
Contributor Author

Maistho commented Oct 5, 2017

@Toxicable

Yup, but I need to specify the generic twice:

this.db.list<MyInterface>('/stuff')
  .valueChanges<MyInterface>();

Whereas this is much more preferable:

this.db.list<MyInterface>('/stuff')
  .valueChanges(); // currently returns Observable<{}[]>

@Maistho
Copy link
Contributor Author

Maistho commented Oct 5, 2017

Also this is "valid" code currently:

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

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

stuff.push(1); // But I can push a number?

@Toxicable
Copy link

Yup, but I need to specify the generic twice:

No you don't see here, https://stackblitz.com/edit/angular-template-firebase-uxldsj?file=app%2Fapp.module.ts
var items is typed to Observable<Item[]>

if you second example you're overriding the generic with another, which is fine.
stuff$ will be typed as a generic of stringbutstuffis still typed as a generic ofnumber`, I don't see an issue here

@Maistho
Copy link
Contributor Author

Maistho commented Oct 6, 2017

@Toxicable Your example is for Cloud Firestore, not Realtime Database.

If I modify your example to use the Realtime Database, it fails.

https://stackblitz.com/edit/angular-template-firebase-7mn3ay

@Maistho
Copy link
Contributor Author

Maistho commented Oct 6, 2017

Also, Firestore does not have a generic specific for collection.valueChanges, but rather uses the one already on the collection.

https://github.com/angular/angularfire2/blob/master/src/firestore/collection/collection.ts#L98

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