Feature Request: Make DocumentReference type generic

[nathanosdev]

The DocumentReference type is wrapping the same type from firestore in a way that hides the generic type.

// original class
export class DocumentReference<T = DocumentData> {
  // ...
}

// angularfire's proxied type
export declare type DocumentReference = firestore.DocumentReference;

I would suggest also proxying the generic parameter like so:

export declare type DocumentReference<T = DocumentData> = firestore.DocumentReference<T>;

I didn't check everywhere that this model is used, but usages would also need to be adjusted. For example:

// current implementation
export class AngularFirestoreCollection<T = DocumentData> {
  add(data: T): Promise<DocumentReference>;
}

// my suggested change
export class AngularFirestoreCollection<T = DocumentData> {
  add(data: T): Promise<DocumentReference<T>>;
}

This would allow for much better type safety when working with the DocumentReference model.

[yasinkocak]

can u make PR?

[nathanosdev]

I could make a PR over the weekend, but it would be nice to have confirmation from the author's of the library that they will support such a change.

[KingDarBoja]

This issue is also related to these request: #2361 and #2438 .

At the end, the types need some fix to provide the correct generics. I think I could submit a PR, need to checkout what's need.

UPDATE: I don't think changing the angularfire functions with the proper return type would solve this as the underlying types from the firebase-js-sdk are not generic as well. Please see packages/firestore-types/index.d.ts and notice how most of them have the DocumentData instead of generics.

[BianchiSeb]

Hello angularFire team. I literally spend a ton of money in your app for my clients. At least answer us. It’s a shame that core features are broken.
It’s a shame you don’t manage the PR.
At least give us back the projets lead and we can manage it like a TRUE open source project.

[nathanosdev]

@KingDarBoja thanks for checking out the Firestore types. I understand your concern, but after looking into this a bit deeper I'm confident that it will work.

For #2361 we need to make the change here in the constructor:

angularfire/src/firestore/document/document.ts

Line 37 in 7cb6c03

constructor(public ref: DocumentReference, private afs: AngularFirestore) { }

We can take the generic parameter from the AngularFirestoreDocument class and give it to the DocumentReference type in the constructor.

constructor(public ref: DocumentReference<T>, private afs: AngularFirestore) { }

The DocumentReference.get method in question will then use that generic type.

get(options?: GetOptions): Promise<DocumentSnapshot<T>>;

https://github.com/firebase/firebase-js-sdk/blob/9a9a81fe4f001f23e9fe1db054c2e7159fca3ae3/packages/firestore-types/index.d.ts#L226

so then the AngularFirestoreDocument.get method will be able to correctly infer the generic return type.

angularfire/src/firestore/document/document.ts

Line 95 in 7cb6c03

return from(this.ref.get(options)).pipe(

And for the other issue you linked: #2438

The Query type from Firestore is generic: https://github.com/firebase/firebase-js-sdk/blob/9a9a81fe4f001f23e9fe1db054c2e7159fca3ae3/packages/firestore-types/index.d.ts#L302

So similar to the previous issue, we can provide a generic type to that type in the constructor:

angularfire/src/firestore/collection/collection.ts

Line 53 in c57cef4

private readonly query: Query,

private readonly query: Query<T>, 

and then when we call AngularFirestoreCollection.get it will be able to correctly infer the return type.

angularfire/src/firestore/collection/collection.ts

Line 125 in c57cef4

return from(this.query.get(options)).pipe(

@BianchiSeb I couldn't agree more. I've done the research into how to achieve this and the 2 other issues that have been linked here. I am ready to make a PR for this, but looking at the history of issues and PRs, it does not look like my PR would go anywhere, which is very disheartening.

[KingDarBoja]

@AnimaMundi Thanks for sharing the solution to this issue, I remember trying to fix it by providing the generics at the interfaces.ts definition but always got an error regarding T being same name but probably being different type.

Anyway, I suggest to submit the PR and the community should bring support to get it merged (thumbs up on the topic), it is better than nothing.

[jamesdaniels]

Should be addressed in 6.1.0-rc.1