Don't splat in snapToData (#33)

* Don't splat in `snapToData` so classes returned by a converter aren't thrown away * `snapToData` should also handle null and non-objects that might be return by converters * Bump versions * Add tests back into canary/release script * Addressed some flaky tests * yarn.lock was showing in the github diff, mark as generated
FirebaseExtended · Sep 9, 2021 · 134e497 · 134e497
1 parent 494dfc0
commit 134e497
Show file tree

Hide file tree

Showing 9 changed files with 191 additions and 91 deletions.
diff --git a/.gitattributes b/.gitattributes
@@ -0,0 +1 @@
+yarn.lock linguist-generated=true
diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml
@@ -125,7 +125,7 @@ jobs:
   publish:
     runs-on: ubuntu-latest
     name: Publish (NPM)
-    needs: ['build']
+    needs: ['build', 'test']
     if: ${{ github.ref == 'refs/heads/main' || github.event_name == 'release' }}
     steps:
       - name: Setup node

diff --git a/firestore/document/index.ts b/firestore/document/index.ts
@@ -44,12 +44,13 @@ export function snapToData<T=DocumentData>(
       idField?: string,
     }={}
 ): {} | undefined {
+  // TODO clean up the typings
+  const data = snapshot.data() as any;
   // match the behavior of the JS SDK when the snapshot doesn't exist
-  if (!snapshot.exists()) {
-    return snapshot.data();
+  // it's possible with data converters too that the user didn't return an object
+  if (!snapshot.exists() || typeof data !== 'object' || data === null) {
+    return data;
   }
-  return {
-    ...snapshot.data(),
-    ...(options.idField ? { [options.idField]: snapshot.id } : null)
-  };
+  if (options.idField) { data[options.idField] = snapshot.id; }
+  return data;
 }
diff --git a/firestore/lite/document/index.ts b/firestore/lite/document/index.ts
@@ -44,12 +44,13 @@ export function snapToData<T=DocumentData>(
       idField?: string,
     }={}
 ): {} | undefined {
+  // TODO clean up the typings
+  const data = snapshot.data() as any;
   // match the behavior of the JS SDK when the snapshot doesn't exist
-  if (!snapshot.exists()) {
-    return snapshot.data();
+  // it's possible with data converters too that the user didn't return an object
+  if (!snapshot.exists() || typeof data !== 'object' || data === null) {
+    return data;
   }
-  return {
-    ...snapshot.data(),
-    ...(options.idField ? { [options.idField]: snapshot.id } : null)
-  };
+  if (options.idField) { data[options.idField] = snapshot.id; }
+  return data;
 }
diff --git a/package.json b/package.json
@@ -1,6 +1,6 @@
 {
   "name": "rxfire",
-  "version": "6.0.0",
+  "version": "6.0.1",
   "private": true,
   "description": "Firebase JavaScript library RxJS",
   "author": "Firebase <[email protected]> (https://firebase.google.com/)",
@@ -73,7 +73,7 @@
     "build:docs": "cp README.md ./dist/ && cp -r ./docs ./dist/",
     "dev": "rollup -c -w",
     "echo:chrome": "echo 'Open Chrome DevTools: \nchrome://inspect/#devices'",
-    "test": "FIREBASE_CLI_PREVIEWS=storageemulator firebase emulators:exec jest --project=rxfire-test-c497c",
+    "test": "FIREBASE_CLI_PREVIEWS=storageemulator firebase emulators:exec \"jest --detectOpenHandles\" --project=rxfire-test-c497c ",
     "test:debug": "yarn echo:chrome && FIREBASE_CLI_PREVIEWS=storageemulator firebase emulators:exec ./test-debug.sh --project=rxfire-test-c497c"
   },
   "dependencies": {

diff --git a/test/database.test.ts b/test/database.test.ts
@@ -316,15 +316,19 @@ describe('RxFire Database', () => {
       it('should process a new child_added event', done => {
         const aref = builtRef(rando());
         const obs = list(aref, { events: [ListenEvent.added] });
-        obs
-          .pipe(skip(2), take(1))
-          .subscribe(changes => {
-            const data = changes.map(change => change.snapshot.val());
-            expect(data).toContainEqual({ name: 'anotha one' });
-          })
-          .add(done);
         set(aref, itemsObj).then(() => {
-          push(aref, { name: 'anotha one' });
+          let count = 0;
+          obs
+          .pipe(take(2))
+          .subscribe(changes => {
+            if (count++ === 0) {
+              push(aref, { name: 'anotha one' });
+            } else {
+              const data = changes.map(change => change.snapshot.val());
+              expect(data).toContainEqual({ name: 'anotha one' });
+              done();
+            }
+          });
         });
       });
 

diff --git a/test/firestore-lite.test.ts b/test/firestore-lite.test.ts
@@ -28,9 +28,9 @@ import {
   docData,
   collectionData,
 } from '../dist/firestore/lite';
-import {map, take, skip} from 'rxjs/operators';
+import { map } from 'rxjs/operators';
 import { default as TEST_PROJECT, firestoreEmulatorPort } from './config';
-import { doc as firestoreDoc, getDocs, collection as firestoreCollection, getDoc, Firestore as FirebaseFirestore, CollectionReference, getFirestore, DocumentReference, connectFirestoreEmulator, doc, setDoc, collection as baseCollection } from 'firebase/firestore/lite';
+import { doc as firestoreDoc, getDocs, collection as firestoreCollection, getDoc, Firestore as FirebaseFirestore, CollectionReference, getFirestore, DocumentReference, connectFirestoreEmulator, doc, setDoc, collection as baseCollection, QueryDocumentSnapshot } from 'firebase/firestore/lite';
 import { initializeApp, deleteApp, FirebaseApp } from 'firebase/app';
 
 const createId = (): string => Math.random().toString(36).substring(5);
@@ -105,6 +105,46 @@ describe('RxFire firestore/lite', () => {
     });
   });
 
+
+  describe('collection w/converter', () => {
+    /**
+     * This is a simple test to see if the collection() method
+     * correctly emits snapshots.
+     *
+     * The test seeds two "people" into the collection. RxFire
+     * creats an observable with the `collection()` method and
+     * asserts that the two "people" are in the array.
+     */
+    it('should emit snapshots', async (done: jest.DoneCallback) => {
+      const {colRef, expectedNames} = await seedTest(firestore);
+
+      class Folk {
+        constructor(public name: string) { }
+        static fromFirestore(snap: QueryDocumentSnapshot) {
+          const name = snap.data().name;
+          if (name !== 'Shannon') {
+            return new Folk(`${snap.data().name}!`);
+          } else {
+            return undefined;
+          }
+        }
+        static toFirestore() {
+          return {};
+        }
+        static collection = colRef.withConverter(Folk);
+      }
+
+      collection(Folk.collection)
+          .subscribe(docs => {
+            const names = docs.map(doc => doc.data()?.name);
+            const classes = docs.map(doc => doc.data()?.constructor?.name);
+            expect(names).toEqual(['David!', undefined]);
+            expect(classes).toEqual(['Folk', undefined]);
+            done();
+          });
+    });
+  });
+
   describe('Data Mapping Functions', () => {
     /**
      * The `unwrap(id)` method will map a collection to its data payload and map the doc ID to a the specificed key.

diff --git a/test/firestore.test.ts b/test/firestore.test.ts
@@ -33,7 +33,7 @@ import {
 } from '../dist/firestore';
 import {map, take, skip} from 'rxjs/operators';
 import { default as TEST_PROJECT, firestoreEmulatorPort } from './config';
-import { getDocs, collection as firestoreCollection, getDoc, DocumentReference, doc as firestoreDoc, Firestore as FirebaseFirestore, CollectionReference, getFirestore, updateDoc, connectFirestoreEmulator, doc, setDoc, DocumentChange, collection as baseCollection } from 'firebase/firestore';
+import { getDocs, collection as firestoreCollection, getDoc, DocumentReference, doc as firestoreDoc, Firestore as FirebaseFirestore, CollectionReference, getFirestore, updateDoc, connectFirestoreEmulator, doc, setDoc, DocumentChange, collection as baseCollection, QueryDocumentSnapshot } from 'firebase/firestore';
 import { initializeApp, deleteApp, FirebaseApp } from 'firebase/app';
 
 const createId = (): string => Math.random().toString(36).substring(5);
@@ -115,6 +115,44 @@ describe('RxFire Firestore', () => {
     });
   });
 
+  describe('collection w/converter', () => {
+    /**
+     * This is a simple test to see if the collection() method
+     * correctly emits snapshots.
+     *
+     * The test seeds two "people" into the collection. RxFire
+     * creats an observable with the `collection()` method and
+     * asserts that the two "people" are in the array.
+     */
+    it('should emit snapshots', (done: jest.DoneCallback) => {
+      const {colRef, expectedNames} = seedTest(firestore);
+
+      class Folk {
+        constructor(public name: string) { }
+        static fromFirestore(snap: QueryDocumentSnapshot) {
+          const name = snap.data().name;
+          if (name !== 'Shannon') {
+            return new Folk(`${snap.data().name}!`);
+          } else {
+            return undefined;
+          }
+        }
+        static toFirestore() {
+          return {};
+        }
+      }
+
+      collection(colRef.withConverter(Folk))
+          .subscribe(docs => {
+            const names = docs.map(doc => doc.data()?.name);
+            const classes = docs.map(doc => doc.data()?.constructor?.name);
+            expect(names).toEqual(['David!', undefined]);
+            expect(classes).toEqual(['Folk', undefined]);
+            done();
+          });
+    });
+  });
+
   describe('collectionChanges', () => {
     /**
      * The `stateChanges()` method emits a stream of events as they