Treating onabort as an error (#823)

packages/firestore/package.json

@@ -40,6 +40,7 @@
     "@types/mocha": "5.0.0",
     "@types/sinon": "4.3.1",
     "chai": "4.1.2",
+    "chai-as-promised": "7.1.1",
     "indexeddbshim": "3.6.2",
     "karma": "2.0.0",
     "karma-chrome-launcher": "2.2.0",

packages/firestore/src/core/firestore_client.ts

@@ -56,6 +56,9 @@ import { ViewSnapshot } from './view_snapshot';
 
 const LOG_TAG = 'FirestoreClient';
 
+/** The DOMException code for quota exceeded. */
+const DOM_EXCEPTION_QUOTA_EXCEEDED = 22;
+
 /**
  * FirestoreClient is a top-level class that constructs and owns all of the
  * pieces of the client SDK architecture. It is responsible for creating the
@@ -199,7 +202,7 @@ export class FirestoreClient {
     if (usePersistence) {
       return this.startIndexedDbPersistence()
         .then(persistenceResult.resolve)
-        .catch((error: FirestoreError) => {
+        .catch(error => {
           // Regardless of whether or not the retry succeeds, from an user
           // perspective, offline persistence has failed.
           persistenceResult.reject(error);
@@ -226,11 +229,24 @@ export class FirestoreClient {
     }
   }
 
-  private canFallback(error: FirestoreError): boolean {
-    return (
-      error.code === Code.FAILED_PRECONDITION ||
-      error.code === Code.UNIMPLEMENTED
-    );
+  /**
+   * Decides whether the provided error allows us to gracefully disable
+   * persistence (as opposed to crashing the client).
+   */
+  private canFallback(error: FirestoreError | DOMException): boolean {
+    if (error instanceof FirestoreError) {
+      return (
+        error.code === Code.FAILED_PRECONDITION ||
+        error.code === Code.UNIMPLEMENTED
+      );
+    } else if (
+      typeof DOMException !== 'undefined' &&
+      error instanceof DOMException
+    ) {
+      return error.code === DOM_EXCEPTION_QUOTA_EXCEEDED;
+    }
+
+    return true;
   }
 
   /**

packages/firestore/src/local/simple_db.ts

@@ -20,6 +20,7 @@ import { AnyDuringMigration } from '../util/misc';
 
 import { PersistencePromise } from './persistence_promise';
 import { SCHEMA_VERSION } from './indexeddb_schema';
+import { Deferred } from '../util/promise';
 
 const LOG_TAG = 'SimpleDb';
 
@@ -145,10 +146,8 @@ export class SimpleDb {
     const transaction = SimpleDbTransaction.open(this.db, mode, objectStores);
     const transactionFnResult = transactionFn(transaction)
       .catch(error => {
-        // Abort the transaction if there was an
-        // error.
-        transaction.abort();
-        return PersistencePromise.reject(error);
+        // Abort the transaction if there was an error.
+        transaction.abort(error);
       })
       .toPromise();
 
@@ -246,11 +245,8 @@ export class SimpleDbTransaction {
 
   /**
    * A promise that resolves with the result of the IndexedDb transaction.
-   *
-   * Note: A transaction explicitly aborted via abort() is considered successful
-   * and this promise will resolve as successful.
    */
-  readonly completionPromise: Promise<void>;
+  private readonly completionDeferred = new Deferred<void>();
 
   static open(
     db: IDBDatabase,
@@ -263,21 +259,36 @@ export class SimpleDbTransaction {
   }
 
   constructor(private readonly transaction: IDBTransaction) {
-    this.completionPromise = new Promise<void>((resolve, reject) => {
-      // We consider aborting to be "normal" and just resolve the promise.
-      // May need to revisit if/when we actually need to abort transactions.
-      this.transaction.onabort = this.transaction.oncomplete = event => {
-        resolve();
-      };
-      this.transaction.onerror = (event: Event) => {
-        reject((event.target as IDBRequest).error);
-      };
-    });
+    this.transaction.oncomplete = () => {
+      this.completionDeferred.resolve();
+    };
+    this.transaction.onabort = () => {
+      if (transaction.error) {
+        this.completionDeferred.reject(transaction.error);
+      } else {
+        this.completionDeferred.resolve();
+      }
+    };
+    this.transaction.onerror = (event: Event) => {
+      this.completionDeferred.reject((event.target as IDBRequest).error);
+    };
   }
 
-  abort(): void {
+  get completionPromise(): Promise<void> {
+    return this.completionDeferred.promise;
+  }
+
+  abort(error?: Error): void {
+    if (error) {
+      this.completionDeferred.reject(error);
+    }
+
     if (!this.aborted) {
-      debug(LOG_TAG, 'Aborting transaction.');
+      debug(
+        LOG_TAG,
+        'Aborting transaction: %s',
+        error ? error.message : 'Client-initiated abort'
+      );
       this.aborted = true;
       this.transaction.abort();
     }

packages/firestore/test/unit/local/simple_db.test.ts

@@ -14,6 +14,9 @@
  * limitations under the License.
  */
 
+import * as chai from 'chai';
+import * as chaiAsPromised from 'chai-as-promised';
+
 import { expect } from 'chai';
 import { SimpleDb } from '../../../src/local/simple_db';
 
@@ -23,6 +26,8 @@ import {
   SimpleDbTransaction
 } from '../../../src/local/simple_db';
 
+chai.use(chaiAsPromised);
+
 interface User {
   id: number;
   name: string;
@@ -130,7 +135,7 @@ describe('SimpleDb', () => {
   it('lets you explicitly abort transactions', async () => {
     await runTransaction((store, txn) => {
       return store.put(dummyUser).next(() => {
-        txn.abort(); // JUST KIDDING!
+        txn.abort();
       });
     });
 
@@ -142,18 +147,13 @@ describe('SimpleDb', () => {
   });
 
   it('aborts transactions when an error happens', async () => {
-    let gotError = false;
-    try {
-      await runTransaction(store => {
+    await expect(
+      runTransaction(store => {
         return store.put(dummyUser).next(() => {
-          throw new Error('error');
+          throw new Error('Generated error');
         });
-      });
-    } catch (error) {
-      expect(error.message).to.equal('error');
-      gotError = true;
-    }
-    expect(gotError).to.equal(true);
+      })
+    ).to.eventually.be.rejectedWith('Generated error');
 
     await runTransaction(store => {
       return store.get(dummyUser.id).next(user => {
@@ -162,20 +162,30 @@ describe('SimpleDb', () => {
     });
   });
 
-  it('still propagates error if you throw after aborting an exception.', async () => {
-    let gotError = false;
-    try {
-      await runTransaction((store, txn) => {
+  it('aborts transactions when persistence promise is rejected', async () => {
+    await expect(
+      runTransaction(store => {
         return store.put(dummyUser).next(() => {
-          txn.abort();
-          throw new Error('error');
+          return PersistencePromise.reject(new Error('Generated error'));
         });
+      })
+    ).to.eventually.be.rejectedWith('Generated error');
+
+    await runTransaction(store => {
+      return store.get(dummyUser.id).next(user => {
+        expect(user).to.deep.equal(null);
       });
-    } catch (error) {
-      expect(error.message).to.equal('error');
-      gotError = true;
-    }
-    expect(gotError).to.equal(true);
+    });
+  });
+
+  it('exposes error from inside a transaction', async () => {
+    await expect(
+      runTransaction(store => {
+        return store.put(dummyUser).next(() => {
+          throw new Error('Generated error');
+        });
+      }).then(() => {}, error => Promise.reject(error))
+    ).to.eventually.be.rejectedWith('Generated error');
 
     await runTransaction(store => {
       return store.get(dummyUser.id).next(user => {

scripts/release/cli.js

@@ -207,7 +207,11 @@ const { argv } = require('yargs');
     /**
      * Release new versions to NPM
      */
-    await publishToNpm(updates, releaseType, argv.canary ? 'canary': 'default');
+    await publishToNpm(
+      updates,
+      releaseType,
+      argv.canary ? 'canary' : 'default'
+    );
 
     /**
      * If this wasn't a production release there

yarn.lock

@@ -1604,6 +1604,12 @@ center-align@^0.1.1:
     align-text "^0.1.3"
     lazy-cache "^1.0.3"
 
+[email protected]:
+  version "7.1.1"
+  resolved "https://registry.npmjs.org/chai-as-promised/-/chai-as-promised-7.1.1.tgz#08645d825deb8696ee61725dbf590c012eb00ca0"
+  dependencies:
+    check-error "^1.0.2"
+
 [email protected]:
   version "4.1.2"
   resolved "https://registry.npmjs.org/chai/-/chai-4.1.2.tgz#0f64584ba642f0f2ace2806279f4f06ca23ad73c"
@@ -1641,7 +1647,7 @@ chardet@^0.4.0:
   version "0.4.2"
   resolved "https://registry.npmjs.org/chardet/-/chardet-0.4.2.tgz#b5473b33dc97c424e5d98dc87d55d4d8a29c8bf2"
 
-check-error@^1.0.1:
+check-error@^1.0.1, check-error@^1.0.2:
   version "1.0.2"
   resolved "https://registry.npmjs.org/check-error/-/check-error-1.0.2.tgz#574d312edd88bb5dd8912e9286dd6c0aed4aac82"