Enforce fixed collection ref on documents

README.md

@@ -6,20 +6,33 @@
     - [Document](#Document)
     - [Collection](#Collection)
   - [API](#API)
+    Sourcing](#Restrictions-on-Dynamic-Data-Sourcing)
+    - [Document](#Document)
+    - [Collection](#Collection)
+  - [API](#API) Sourcing](#Restrictions-on-Dynamic-Data-Sourcing)
+    - [Document](#Document)
+    - [Collection](#Collection)
+  - [API](#API) Sourcing](#Restrictions-on-Dynamic-Data-Sourcing)
+    - [Document](#Document)
+    - [Collection](#Collection)
+  - [API](#API)
 
 # Firestore MobX
 
-This library was inspired by [Firestorter](https://github.com/IjzerenHein/firestorter). Read the [migration
+**WARNING** Do not use this yet. There are still some fundamental issues to be
+solved and breaking API changes are likely to happen at any time.
+
+This library was inspired by
+[Firestorter](https://github.com/IjzerenHein/firestorter). Read the [migration
 docs](/docs/migrate-from-firestorter.md) if you are interested in the motivation
 and differences.
 
 You should be able to use this in any Javascript application including React,
 React-Native and Node.js.
 
-**DISCLAIMER** This library is still very new and based on my personal
-experience using Firestorter. If there are any features that you miss and deem
-essential, please let me know. It is well possible that I have overlooked some
-valid use-cases.
+**NOTE** This library is based on my personal experience using Firestorter. If
+there are any features that you miss and deem essential, please let me know. It
+is well possible that I have overlooked some valid use-cases.
 
 ## Features
 
@@ -41,27 +54,31 @@ offer strong typing some restrictions are enforced.
 
 ### Document
 
-1. An observable document can change its ref after it was created, but the new
-   ref needs to be from the same collection. This is required because with
-   Typescript we get compile-time type checks based on what you pass into the
-   constructor. If the ref would be allowed to switch to a different collection,
-   this type would have no practical meaning. Also I have to yet encounter a
-   situation that requires this in a real-life application.
+1. An observable document always links to the Firestore collection passed into
+   the constructor. A document can change its id after it was created, switching
+   to a different document in Firestore, but the collection reference will never
+   change. With Typescript we get compile-time type checks based on the schema
+   you use to declare the instance with. If the source would be allowed to
+   switch to a different collection this type would have no practical meaning.
+
+   Also I do not think there is a need for this in a real-life application. If
+   you need to observe documents from different collections just create
+   multiple ObservableDocument instances.
 
 ### Collection
 
 1. An observable collection always links to the Firestore collection passed into
    the constructor. The query can be changed after the object was created,
    influencing the number of documents in the collection, but it can not switch
-   to a different collection dynamically. The motivation for this is similar to
+   to a different collection dynamically. The motivation for this is the same as
    restriction 1 on observable documents.
 
 2. A collection without a query produces no documents. Retrieving all documents
    from a collection is not typically something you would do in a client-side
    application. By placing this restriction on collections it not only
-   simplifies the logic but we avoid fetching a large collection by accident. If
+   simplifies the logic but we can avoid fetching a large collection by accident. If
    you have a relatively small collection and you do want to fetch all of it,
-   you can simply pass in a Firestore query that would include everything, for
+   you can simply pass in a Firestore query that would include all documents. For
    example `.orderBy("updatedAt", "desc")`, `.limit(999)` or `.after("0")`
 
 ## API

docs/todo.md

@@ -1,10 +1,12 @@
 # TODO version 1.0
 
+## Bugs
+
+- Is there a clash between observable collections when different queries are used on the same collection?
+
 ## Must Have
 
-- Figure out what to do with refs of sub-collections. Possibly remove restrictions.
-- Limit document ref changes to same collection, otherwise T doesn't make any
-  sense anymore.
+- Allow collection to switch between same sub-collections.
 - Add tests
 
 ## Should Have

package.json

@@ -14,7 +14,6 @@
   "scripts": {
     "build": "rollup -c",
     "build:watch": "rollup -cw",
-    "deploy": "np",
     "test": "echo \"Error: no test specified\" && exit 0",
     "lint": "eslint 'src/**/*.{js,ts,tsx}' --quiet --fix",
     "format": "prettier --write \"src/**/*.{ts,tsx,md,json,yml}\" \"*.{js,json,md,yml}\""

src/document.ts

@@ -18,21 +18,35 @@ export interface Document<T> {
   ref: firestore.DocumentReference;
 }
 
-function isReference<T>(
-  source: firestore.DocumentReference | Document<T>
+function isDocumentReference<T>(
+  source: SourceType<T>
 ): source is firestore.DocumentReference {
   return (source as firestore.DocumentReference).path !== undefined;
 }
 
+function isCollectionReference<T>(
+  source: SourceType<T>
+): source is firestore.CollectionReference {
+  return (source as firestore.CollectionReference).id !== undefined;
+}
+
+function getPathFromCollectionRef(
+  collectionRef: firestore.CollectionReference
+) {
+  return `${collectionRef.id}/__no_document_id`;
+}
+
+type SourceType<T> =
+  | firestore.DocumentReference
+  | firestore.CollectionReference
+  | Document<T>;
+
 export class ObservableDocument<T extends object> {
   @observable private dataObservable: IObservableValue<T | undefined>;
   @observable private isLoadingObservable: IObservableValue<boolean>;
-  // private dataObservable: IObservableValue<T | undefined> = observable({});
-  // private isLoadingObservable: IObservableValue<boolean> = observable.box(
-  //   false
-  // );
 
   private _ref?: firestore.DocumentReference;
+  private _collectionRef: firestore.CollectionReference;
   private isDebugEnabled = false;
   private _path?: string;
   private _exists = false;
@@ -41,10 +55,7 @@ export class ObservableDocument<T extends object> {
   private onSnapshotUnsubscribeFn?: () => void;
   private options: Options = {};
 
-  public constructor(
-    source?: firestore.DocumentReference | Document<T>,
-    options?: Options
-  ) {
+  public constructor(source: SourceType<T>, options?: Options) {
     this.dataObservable = observable.box();
     this.isLoadingObservable = observable.box(false);
 
@@ -53,14 +64,21 @@ export class ObservableDocument<T extends object> {
       this.isDebugEnabled = options.debug || false;
     }
 
-    if (!source) {
-      // There is nothing to initialize really
-    } else if (isReference<T>(source)) {
+    if (isCollectionReference<T>(source)) {
+      this._collectionRef = source;
+      this._path = getPathFromCollectionRef(source);
+      runInAction(() => this.updateListeners(true));
+    } else if (isDocumentReference<T>(source)) {
       this._ref = source;
+      this._collectionRef = source.parent;
       this._path = source.path;
       runInAction(() => this.updateListeners(true));
     } else {
+      /**
+       * Source is type Document, passed in from an already existing snapshot
+       */
       this._ref = source.ref;
+      this._collectionRef = source.ref.parent;
       this._path = source.ref.path;
 
       runInAction(() => {
@@ -75,10 +93,18 @@ export class ObservableDocument<T extends object> {
     onBecomeUnobserved(this, "dataObservable", this.suspendUpdates);
   }
 
-  public get id() {
+  public get id(): string | undefined {
     return this._ref ? this._ref.id : undefined;
   }
 
+  public set id(documentId: string | undefined) {
+    if (this.id === documentId) {
+      return;
+    }
+
+    runInAction(() => this.changeDocumentId(documentId));
+  }
+
   public get data() {
     return this.dataObservable.get();
   }
@@ -105,20 +131,6 @@ export class ObservableDocument<T extends object> {
     return this._ref;
   }
 
-  public set ref(ref: firestore.DocumentReference | undefined) {
-    /**
-     * If the ref is the same as current it is a no-op
-     */
-    if (ref && this.ref && this.ref.path === ref.path) {
-      return;
-    }
-
-    /**
-     * @TODO check if new ref is in the same collection, otherwise T doesn't makes sense anymore
-     */
-    runInAction(() => this.changeRef(ref));
-  }
-
   public update(fields: firestore.UpdateData): Promise<void> {
     if (!this._ref) {
       throw Error("Can not update data on document with undefined ref");
@@ -190,15 +202,17 @@ export class ObservableDocument<T extends object> {
     throw new Error(`${this.path} onSnapshotError: ${err.message}`);
   }
 
-  private changeRef(ref: firestore.DocumentReference | undefined) {
-    // @TODO generate unique id
-    const newPath = ref ? ref.path : "__no_source";
+  private changeDocumentId(documentId?: string) {
+    const newRef = documentId ? this._collectionRef.doc(documentId) : undefined;
+    const newPath = newRef
+      ? newRef.path
+      : getPathFromCollectionRef(this._collectionRef);
 
     this.logDebug(`Switch source to ${newPath}`);
-    this._ref = ref;
+    this._ref = newRef;
     this._path = newPath;
 
-    const hasSource = !!ref;
+    const hasSource = !!newRef;
     const wasListening = !!this.onSnapshotUnsubscribeFn;
 
     if (wasListening) {