-
Notifications
You must be signed in to change notification settings - Fork 398
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
Add useFirestoreCollectionDataOnce #252
base: main
Are you sure you want to change the base?
Add useFirestoreCollectionDataOnce #252
Conversation
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed (or fixed any issues), please reply here with What to do if you already signed the CLAIndividual signers
Corporate signers
ℹ️ Googlers: Go here for more info. |
@googlebot I signed it! |
CLAs look good, thanks! ℹ️ Googlers: Go here for more info. |
const idField = checkIdField(options); | ||
const queryId = `firestore:collectionDataOnce:${getUniqueIdForFirestoreQuery(query)}:idField=${idField}`; | ||
|
||
return useObservable(collectionData(query, idField).pipe(first()), queryId, checkStartWithValue(options)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is tricky. I'm not sure if using first()
to terminate an onSnapshot()
listener is the right call when we could use .get()
which is meant for one time reads.
What's tricky is that we don't support this natively in RxFire and ReactFire does not take a dependency of RxJS. This means I don't think we can directly create an observable from a promise like we would below in RxFire.
function getCollectionData(colRef) {
return from(citiesCol.get());
}
So maybe we just add that to RxFire and then add that here? What are you thoughts? I'm happy to make the PR to RxFire or if you'd like to let me know as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the insight!
we could use .get() which is meant for one time reads.
I totally agree.
ReactFire does not take a dependency of RxJS.
Maybe I don't understand it properly, but it looks like it does have the dependency on rxjs.
So it looks like I could write the following if I want, but maybe it's just not a good thing design-wise for the reactfire and rxfire libraries (I'm not sure).
diff --git a/reactfire/firestore/index.tsx b/reactfire/firestore/index.tsx
index 0974b14..14971b1 100644
--- a/reactfire/firestore/index.tsx
+++ b/reactfire/firestore/index.tsx
@@ -1,9 +1,10 @@
import { firestore } from 'firebase/app';
-import { collectionData, doc, docData, fromCollectionRef } from 'rxfire/firestore';
+import { collectionData, doc, docData, fromCollectionRef, snapToData } from 'rxfire/firestore';
import { preloadFirestore, ReactFireOptions, useObservable, checkIdField, checkStartWithValue } from '..';
import { preloadObservable } from '../useObservable';
-import { first } from 'rxjs/operators';
+import { map, first } from 'rxjs/operators';
import { useFirebaseApp } from '../firebaseApp';
+import { from } from 'rxjs';
const CACHED_QUERIES = '_reactFireFirestoreQueryCache';
@@ -124,6 +125,6 @@ export function useFirestoreCollectionData<T = { [key: string]: unknown }>(query
export function useFirestoreCollectionDataOnce<T = { [key: string]: unknown }>(query: firestore.Query, options?: ReactFireOptions<T[]>): T[] {
const idField = checkIdField(options);
const queryId = `firestore:collectionDataOnce:${getUniqueIdForFirestoreQuery(query)}:idField=${idField}`;
-
- return useObservable(collectionData(query, idField).pipe(first()), queryId, checkStartWithValue(options));
+ const onetimeCollectionData = from(query.get()).pipe(map(snapshot => snapshot.docs.map(snap => snapToData(snap, idField) as T)));
+ return useObservable(onetimeCollectionData, queryId, checkStartWithValue(options));
}
If you'd like me to put some logic on the rxfire side, I'd be happy to do so! Please let me know. Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@davideast and @banyan any update on this?
It would be a great addition to reactfire.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @banyan!
Thank you so much for the PR with the docs and the tests! I have one small question about the implementation, so let me know your thoughts.
Would be great to have this |
Hey guys. Any progress here? This seems to be pretty handy to have. |
A part of #137
I've implemented this with reference to #211 :)