Improve IDs passed to useObservable (#167)

* add realtime database tests

* better observable ids

* throw an error if no observableid is provided

* fix comment about emulators

* update the docs

docs/reference.md

@@ -104,10 +104,10 @@ _Throws a Promise by default_
 
 #### Parameters
 
-| Parameter   | Type                                                                                                          | Description                                                                  |
-| ----------- | ------------------------------------------------------------------------------------------------------------- | ---------------------------------------------------------------------------- |
-| ref         | [`CollectionReference`](https://firebase.google.com/docs/reference/js/firebase.firestore.CollectionReference) | A reference to the collection you want to listen to                          |
-| options _?_ | ReactFireOptions                                                                                              | Options. This hook will not throw a Promise if you provide `startWithValue`. |
+| Parameter   | Type                                                                              | Description                                                                  |
+| ----------- | --------------------------------------------------------------------------------- | ---------------------------------------------------------------------------- |
+| ref         | [`Query`](https://firebase.google.com/docs/reference/js/firebase.firestore.Query) | A query for the collection you want to listen to                             |
+| options _?_ | ReactFireOptions                                                                  | Options. This hook will not throw a Promise if you provide `startWithValue`. |
 
 #### Returns
 

reactfire/auth/index.tsx

@@ -37,7 +37,7 @@ export function useUser<T = unknown>(
 
   return useObservable(
     user(auth),
-    'user',
+    'auth: user',
     options ? options.startWithValue : undefined
   );
 }

reactfire/database/database.test.tsx

@@ -1,11 +1,140 @@
 import '@testing-library/jest-dom/extend-expect';
+import { render, waitForElement, cleanup, act } from '@testing-library/react';
+import * as React from 'react';
+import '@testing-library/jest-dom/extend-expect';
+import * as firebase from '@firebase/testing';
+import { useDatabaseObject, useDatabaseList, FirebaseAppProvider } from '..';
+import { database } from 'firebase/app';
+import { QueryChange } from 'rxfire/database/dist/database';
 
 describe('Realtime Database (RTDB)', () => {
+  let app: import('firebase').app.App;
+
+  beforeAll(async () => {
+    app = firebase.initializeTestApp({
+      projectId: '12345',
+      databaseName: 'my-database',
+      auth: { uid: 'alice' }
+    }) as import('firebase').app.App;
+  });
+
+  afterEach(async () => {
+    cleanup();
+
+    // clear out the database
+    app
+      .database()
+      .ref()
+      .set(null);
+  });
+
+  test('sanity check - emulator is running', () => {
+    // IF THIS TEST FAILS, MAKE SURE YOU'RE RUNNING THESE TESTS BY DOING:
+    // yarn test
+
+    return app
+      .database()
+      .ref('hello')
+      .set({ a: 'world' });
+  });
+
   describe('useDatabaseObject', () => {
-    test.todo("returns the same value as ref.on('value')");
+    it('can get an object [TEST REQUIRES EMULATOR]', async () => {
+      const mockData = { a: 'hello' };
+
+      const ref = app.database().ref('hello');
+
+      await ref.set(mockData);
+
+      const ReadObject = () => {
+        const { snapshot } = useDatabaseObject(ref);
+
+        return <h1 data-testid="readSuccess">{snapshot.val().a}</h1>;
+      };
+
+      const { getByTestId } = render(
+        <FirebaseAppProvider firebase={app}>
+          <React.Suspense fallback={<h1 data-testid="fallback">Fallback</h1>}>
+            <ReadObject />
+          </React.Suspense>
+        </FirebaseAppProvider>
+      );
+
+      await waitForElement(() => getByTestId('readSuccess'));
+
+      expect(getByTestId('readSuccess')).toContainHTML(mockData.a);
+    });
   });
 
   describe('useDatabaseList', () => {
-    test.todo("returns the same value as ref.on('value')");
+    it('can get a list [TEST REQUIRES EMULATOR]', async () => {
+      const mockData1 = { a: 'hello' };
+      const mockData2 = { a: 'goodbye' };
+
+      const ref = app.database().ref('myList');
+
+      await act(() => ref.push(mockData1));
+      await act(() => ref.push(mockData2));
+
+      const ReadList = () => {
+        const changes = useDatabaseList(ref) as QueryChange[];
+
+        return (
+          <ul data-testid="readSuccess">
+            {changes.map(({ snapshot }) => (
+              <li key={snapshot.key} data-testid="listItem">
+                {snapshot.val().a}
+              </li>
+            ))}
+          </ul>
+        );
+      };
+
+      const { getAllByTestId } = render(
+        <FirebaseAppProvider firebase={app}>
+          <React.Suspense fallback={<h1 data-testid="fallback">Fallback</h1>}>
+            <ReadList />
+          </React.Suspense>
+        </FirebaseAppProvider>
+      );
+
+      await waitForElement(() => getAllByTestId('listItem'));
+
+      expect(getAllByTestId('listItem').length).toEqual(2);
+    });
+
+    it('Returns different data for different queries on the same path [TEST REQUIRES EMULATOR]', async () => {
+      const mockData1 = { a: 'hello' };
+      const mockData2 = { a: 'goodbye' };
+
+      const ref = app.database().ref('items');
+      const filteredRef = ref.orderByChild('a').equalTo('hello');
+
+      await act(() => ref.push(mockData1));
+      await act(() => ref.push(mockData2));
+
+      const ReadFirestoreCollection = () => {
+        const list = useDatabaseList(ref) as QueryChange[];
+        const filteredList = useDatabaseList(filteredRef) as QueryChange[];
+
+        // filteredList's length should be 1 since we only added one value that matches its query
+        expect(filteredList.length).toEqual(1);
+
+        // the full list should be bigger than the filtered list
+        expect(list.length).toBeGreaterThan(filteredList.length);
+
+        return <h1 data-testid="rendered">Hello</h1>;
+      };
+
+      const { getByTestId } = render(
+        <FirebaseAppProvider firebase={app}>
+          <React.Suspense fallback={<h1 data-testid="fallback">Fallback</h1>}>
+            <ReadFirestoreCollection />
+          </React.Suspense>
+        </FirebaseAppProvider>
+      );
+
+      await waitForElement(() => getByTestId('rendered'));
+    });
   });
 });

reactfire/database/index.tsx

@@ -14,11 +14,18 @@ export function useDatabaseObject<T = unknown>(
 ): QueryChange | T {
   return useObservable(
     object(ref),
-    ref.toString(),
+    `RTDB: ${ref.toString()}`,
     options ? options.startWithValue : undefined
   );
 }
 
+// Realtime Database has an undocumented method
+// that helps us build a unique ID for the query
+// https://github.com/firebase/firebase-js-sdk/blob/aca99669dd8ed096f189578c47a56a8644ac62e6/packages/database/src/api/Query.ts#L601
+interface _QueryWithId extends database.Query {
+  queryIdentifier(): string;
+}
+
 /**
  * Subscribe to a Realtime Database list
  *
@@ -29,9 +36,11 @@ export function useDatabaseList<T = { [key: string]: unknown }>(
   ref: database.Reference | database.Query,
   options?: ReactFireOptions<T[]>
 ): QueryChange[] | T[] {
+  const hash = `RTDB: ${ref.toString()}|${(ref as _QueryWithId).queryIdentifier()}`;
+
   return useObservable(
     list(ref),
-    ref.toString(),
+    hash,
     options ? options.startWithValue : undefined
   );
 }

reactfire/firestore/firestore.test.tsx

@@ -1,5 +1,4 @@
-import { renderHook, act } from '@testing-library/react-hooks';
-import { render, waitForElement, cleanup } from '@testing-library/react';
+import { render, waitForElement, cleanup, act } from '@testing-library/react';
 
 import * as React from 'react';
 import '@testing-library/jest-dom/extend-expect';
@@ -32,7 +31,7 @@ describe('Firestore', () => {
 
   test('sanity check - emulator is running', () => {
     // IF THIS TEST FAILS, MAKE SURE YOU'RE RUNNING THESE TESTS BY DOING:
-    //
+    // yarn test
 
     return app
       .firestore()
@@ -105,9 +104,6 @@ describe('Firestore', () => {
     });
   });
 
-  // THIS TEST CAUSES A REACT `act` WARNING
-  // IT WILL BE FIXED IN REACT 16.9
-  // More info here: https://github.com/testing-library/react-testing-library/issues/281
   describe('useFirestoreCollection', () => {
     it('can get a Firestore collection [TEST REQUIRES EMULATOR]', async () => {
       const mockData1 = { a: 'hello' };
@@ -115,8 +111,8 @@ describe('Firestore', () => {
 
       const ref = app.firestore().collection('testCollection');
 
-      await ref.add(mockData1);
-      await ref.add(mockData2);
+      await act(() => ref.add(mockData1));
+      await act(() => ref.add(mockData2));
 
       const ReadFirestoreCollection = () => {
         const collection = useFirestoreCollection(ref);
@@ -143,11 +139,45 @@ describe('Firestore', () => {
 
       expect(getAllByTestId('listItem').length).toEqual(2);
     });
+
+    it('Returns different data for different queries on the same path [TEST REQUIRES EMULATOR]', async () => {
+      const mockData1 = { a: 'hello' };
+      const mockData2 = { a: 'goodbye' };
+
+      const ref = app.firestore().collection('testCollection');
+      const filteredRef = ref.where('a', '==', 'hello');
+
+      await act(() => ref.add(mockData1));
+      await act(() => ref.add(mockData2));
+
+      const ReadFirestoreCollection = () => {
+        const list = (useFirestoreCollection(ref) as firestore.QuerySnapshot)
+          .docs;
+        const filteredList = (useFirestoreCollection(
+          filteredRef
+        ) as firestore.QuerySnapshot).docs;
+
+        // filteredList's length should be 1 since we only added one value that matches its query
+        expect(filteredList.length).toEqual(1);
+
+        // the full list should be bigger than the filtered list
+        expect(list.length).toBeGreaterThan(filteredList.length);
+
+        return <h1 data-testid="rendered">Hello</h1>;
+      };
+
+      const { getByTestId } = render(
+        <FirebaseAppProvider firebase={app}>
+          <React.Suspense fallback={<h1 data-testid="fallback">Fallback</h1>}>
+            <ReadFirestoreCollection />
+          </React.Suspense>
+        </FirebaseAppProvider>
+      );
+
+      await waitForElement(() => getByTestId('rendered'));
+    });
   });
 
-  // THIS TEST CAUSES A REACT `act` WARNING
-  // IT WILL BE FIXED IN REACT 16.9
-  // More info here: https://github.com/testing-library/react-testing-library/issues/281
   describe('useFirestoreCollectionData', () => {
     it('can get a Firestore collection [TEST REQUIRES EMULATOR]', async () => {
       const mockData1 = { a: 'hello' };
@@ -155,8 +185,8 @@ describe('Firestore', () => {
 
       const ref = app.firestore().collection('testCollection');
 
-      await ref.add(mockData1);
-      await ref.add(mockData2);
+      await act(() => ref.add(mockData1));
+      await act(() => ref.add(mockData2));
 
       const ReadFirestoreCollection = () => {
         const list = useFirestoreCollectionData<any>(ref, { idField: 'id' });
@@ -183,5 +213,41 @@ describe('Firestore', () => {
 
       expect(getAllByTestId('listItem').length).toEqual(2);
     });
+
+    it('Returns different data for different queries on the same path [TEST REQUIRES EMULATOR]', async () => {
+      const mockData1 = { a: 'hello' };
+      const mockData2 = { a: 'goodbye' };
+
+      const ref = app.firestore().collection('testCollection');
+      const filteredRef = ref.where('a', '==', 'hello');
+
+      await act(() => ref.add(mockData1));
+      await act(() => ref.add(mockData2));
+
+      const ReadFirestoreCollection = () => {
+        const list = useFirestoreCollectionData<any>(ref, { idField: 'id' });
+        const filteredList = useFirestoreCollectionData<any>(filteredRef, {
+          idField: 'id'
+        });
+
+        // filteredList's length should be 1 since we only added one value that matches its query
+        expect(filteredList.length).toEqual(1);
+
+        // the full list should be bigger than the filtered list
+        expect(list.length).toBeGreaterThan(filteredList.length);
+
+        return <h1 data-testid="rendered">Hello</h1>;
+      };
+
+      const { getByTestId } = render(
+        <FirebaseAppProvider firebase={app}>
+          <React.Suspense fallback={<h1 data-testid="fallback">Fallback</h1>}>
+            <ReadFirestoreCollection />
+          </React.Suspense>
+        </FirebaseAppProvider>
+      );
+
+      await waitForElement(() => getByTestId('rendered'));
+    });
   });
 });

reactfire/firestore/index.tsx

@@ -1,9 +1,9 @@
 import { firestore } from 'firebase/app';
 import {
-  doc,
   collectionData,
-  fromCollectionRef,
-  docData
+  doc,
+  docData,
+  fromCollectionRef
 } from 'rxfire/firestore';
 import { ReactFireOptions, useObservable } from '..';
 
@@ -19,7 +19,7 @@ export function useFirestoreDoc<T = unknown>(
 ): firestore.DocumentSnapshot | T {
   return useObservable(
     doc(ref),
-    ref.path,
+    `firestore: ${ref.path}`,
     options ? options.startWithValue : undefined
   );
 }
@@ -36,7 +36,7 @@ export function useFirestoreDocData<T = unknown>(
 ): T {
   return useObservable(
     docData(ref, checkIdField(options)),
-    ref.path,
+    `firestore: ${ref.path}`,
     checkStartWithValue(options)
   );
 }
@@ -48,16 +48,33 @@ export function useFirestoreDocData<T = unknown>(
  * @param options
  */
 export function useFirestoreCollection<T = { [key: string]: unknown }>(
-  ref: firestore.CollectionReference,
+  query: firestore.Query,
   options?: ReactFireOptions<T[]>
 ): firestore.QuerySnapshot | T[] {
+  const queryId = getHashFromFirestoreQuery(query);
+
   return useObservable(
-    fromCollectionRef(ref),
-    ref.path,
+    fromCollectionRef(query, checkIdField(options)),
+    queryId,
     options ? options.startWithValue : undefined
   );
 }
 
+// The Firestore SDK has an undocumented _query
+// object that has a method to generate a hash for a query,
+// which we need for useObservable
+// https://github.com/firebase/firebase-js-sdk/blob/5beb23cd47312ffc415d3ce2ae309cc3a3fde39f/packages/firestore/src/core/query.ts#L221
+interface _QueryWithId extends firestore.Query {
+  _query: {
+    canonicalId(): string;
+  };
+}
+
+function getHashFromFirestoreQuery(query: firestore.Query) {
+  const hash = (query as _QueryWithId)._query.canonicalId();
+  return `firestore: ${hash}`;
+}
+
 /**
  * Subscribe to a Firestore collection and unwrap the snapshot.
  *
@@ -65,12 +82,14 @@ export function useFirestoreCollection<T = { [key: string]: unknown }>(
  * @param options
  */
 export function useFirestoreCollectionData<T = { [key: string]: unknown }>(
-  ref: firestore.CollectionReference,
+  query: firestore.Query,
   options?: ReactFireOptions<T[]>
 ): T[] {
+  const queryId = getHashFromFirestoreQuery(query);
+
   return useObservable(
-    collectionData(ref, checkIdField(options)),
-    ref.path,
+    collectionData(query, checkIdField(options)),
+    queryId,
     checkStartWithValue(options)
   );
 }

reactfire/package.json

@@ -8,8 +8,8 @@
   "scripts": {
     "build-dev": "tsc --watch",
     "test-dev": "jest --verbose --watch",
-    "emulator": "firebase emulators:start --only firestore",
-    "test": "firebase emulators:exec --only firestore \"jest --no-cache --verbose --detectOpenHandles --forceExit\"",
+    "emulators": "firebase emulators:start --only firestore,database",
+    "test": "firebase emulators:exec --only firestore,database \"jest --no-cache --verbose --detectOpenHandles --forceExit\"",
     "copy-package-json": "cp package.pub.json pub/reactfire/package.json",
     "watch": "yarn build && tsc --watch",
     "build": "rm -rf pub && tsc && yarn copy-package-json && cp ../README.md pub/reactfire/README.md && cp ../LICENSE pub/reactfire/LICENSE"

reactfire/storage/index.tsx

@@ -38,7 +38,7 @@ export function useStorageTask<T = unknown>(
 ): storage.UploadTaskSnapshot | T {
   return useObservable(
     _fromTask(task),
-    'upload' + ref.toString(),
+    'storage upload: ' + ref.toString(),
     options ? options.startWithValue : undefined
   );
 }
@@ -55,7 +55,7 @@ export function useStorageDownloadURL<T = string>(
 ): string | T {
   return useObservable(
     getDownloadURL(ref),
-    'download' + ref.toString(),
+    'storage download:' + ref.toString(),
     options ? options.startWithValue : undefined
   );
 }

reactfire/useObservable/index.ts

@@ -31,6 +31,10 @@ export function useObservable(
   observableId: string,
   startWithValue?: any
 ) {
+  if (!observableId) {
+    throw new Error('cannot call useObservable without an observableId');
+  }
+
   const request = requestCache.getRequest(observable$, observableId);
 
   const initialValue =

reactfire/useObservable/useObservable.test.tsx

@@ -18,6 +18,16 @@ describe('useObservable', () => {
     }
   });
 
+  it('throws an error if no observableId is provided', () => {
+    const observable$: Subject<any> = new Subject();
+
+    try {
+      useObservable(observable$, undefined);
+    } catch (thingThatWasThrown) {
+      expect(thingThatWasThrown).toBeInstanceOf(Error);
+    }
+  });
+
   it('can return a startval and then the observable once it is ready', () => {
     const startVal = 'howdy';
     const observableVal = "y'all";