angular / angularfire

Angular + Firebase = ❤️
https://firebaseopensource.com/projects/angular/angularfire2
MIT License
7.68k stars 2.19k forks source link

AngularFire 7 Document Typing #2931

Open kevin-induro opened 3 years ago

kevin-induro commented 3 years ago

Have we lost document typing with the upgrade to AngularFire 7?

In the past, we were able to do (as shown in the docs) something like afs.doc<Item>('items/1') and that would return DocumentReference<Item> as the type.

Now it looks like the functions that exist are

function doc(firestore: types.FirebaseFirestore, path: string, ...pathSegments: string[]): DocumentReference<DocumentData>;
function doc<T>(reference: types.CollectionReference<T>, path?: string, ...pathSegments: string[]): DocumentReference<T>;
function doc(reference: types.DocumentReference<unknown>, path: string, ...pathSegments: string[]): DocumentReference<DocumentData>;

The only one that is typed is the one that takes in a previous collection reference. Meanwhile, this first is the one we're directed to use in the upgrade documentation and it only uses DocumentData. One of the big benefits to using AngularFire previously over the vanilla Firebase libraries was the fact that DocumentData didn't have to be cast. Has that now been removed?

google-oss-bot commented 3 years ago

This issue does not seem to follow the issue template. Make sure you provide all the required information.

kevin-induro commented 3 years ago

Angular: 12.2.4 Firebase: 9.0.1 AngularFire: 7.0.3

kevin-induro commented 3 years ago

I should also mention that the above means that the current upgrade documentation is wrong. It states that the alternative to doc in v6 / Compat is

import { doc } from '@angular/fire/firestore';
doc<T>(firestore, 'foo/bar') // DocumentReference<T>

In fact, this gives an error stating Argument of type 'Firestore' is not assignable to parameter of type 'CollectionReference<T>'.

jamesdaniels commented 3 years ago

It seems Firestore isn't allowing one to override the typings, only allowing types to be inferred if using a converter. Converters are "better" in the sense that you can have real run-time checks to guard you against data that doesn't conform to your expectations but they can be overly verbose.

const fooCollection = collection(firestore, 'foo').withConverter({
  toFirestore(data) => ({ ...data }),
  fromFirestore(snapshot) => new Foo(..),
})

return collectionData(fooCollection); //=> Observable<Foo[]>

I'll address in a future release of Angular/RxFire.

kevin-induro commented 3 years ago

Would you like to keep this issue open to track here, or should I open another issue in RxFire?

jamesdaniels commented 3 years ago

This is fine, I have a WIP to add better generics support over there.

ex1tium commented 3 years ago

I have been just typecasting manually like so return docData(document) as Observable<AppUser> but I prefer the old way of doing things. Please add better generics support.

Methodician commented 2 years ago

@jamesdaniels does this mean we will eventually get functionality in line with the current documentation, or that the documentation will be updated to reflect the new (seemingly downgraded) doc and collection type setting?

davidecampello commented 2 years ago

I'm having the same issue. Is there a better solution than casting the data manually?

PVermeer commented 2 years ago

When using a service I use this:

private userCollection = collection(this.firestore, 'users') as CollectionReference<FirestoreUser | undefined>;

Then: const docRef = doc(this.userCollection, id); seems to type correctly now.