Adding experimentalAutoDetectLongPolling (#3724)

* Upgrade to latest version of closure

* Adding changeset

* Updating changeset message

* Fixing stylization in changeset copy

* WIP

* Updating to match new patterns

* Making member names match conventions on others

* Making the linter happy

* Fixing bug caught by unit tests and misc polish

* Turning auto detection on by default

* Turning auto detection on by default

* Update packages/firestore/src/util/input_validation.ts

Co-authored-by: Sebastian Schmidt <[email protected]>

* Update packages/firebase/index.d.ts

Co-authored-by: Sebastian Schmidt <[email protected]>

* Changing default to false

* Fixing grammar

* Adding integration test

* Undoing formatting changes

* Resetting hard code to false

* Update packages/firestore/src/core/database_info.ts

Co-authored-by: Sebastian Schmidt <[email protected]>

* Update packages/firestore/test/integration/api/database.test.ts

Co-authored-by: Sebastian Schmidt <[email protected]>

* Create yellow-turkeys-lay.md

* Update .changeset/yellow-turkeys-lay.md

Co-authored-by: Sebastian Schmidt <[email protected]>

* Update .changeset/yellow-turkeys-lay.md

Co-authored-by: Sebastian Schmidt <[email protected]>

* Update index.d.ts

Adding back-ticks for literal

* Update yellow-turkeys-lay.md

Bumping firebase minor version

* Updating unit test to contain proper error string

Co-authored-by: Sebastian Schmidt <[email protected]>

Author: rafikhan

.changeset/yellow-turkeys-lay.md

@@ -0,0 +1,11 @@
+---
+"@firebase/firestore-types": minor
+"@firebase/firestore": minor
+"@firebase/webchannel-wrapper": minor
+"firebase": minor
+---
+
+Adds a new `experimentalAutoDetectLongPolling` to FirestoreSettings.  When
+enabled, the SDK's underlying transport (WebChannel) automatically detects if 
+long-polling should be used. This is very similar to 
+`experimentalForceLongPolling`, but only uses long-polling if required.

packages/firebase/index.d.ts

@@ -7879,15 +7879,27 @@ declare namespace firebase.firestore {
      * buffer traffic indefinitely. Use of this option will cause some
      * performance degradation though.
      *
-     * This setting may be removed in a future release. If you find yourself
-     * using it to work around a specific network reliability issue, please
-     * tell us about it in
-     * https://github.com/firebase/firebase-js-sdk/issues/1674.
+     * This setting cannot be used with `experimentalAutoDetectLongPolling` and
+     * may be removed in a future release. If you find yourself using it to
+     * work around a specific network reliability issue, please tell us about
+     * it in https://github.com/firebase/firebase-js-sdk/issues/1674.
      *
      * @webonly
      */
     experimentalForceLongPolling?: boolean;
 
+    /**
+     * Configures the SDK's underlying transport (WebChannel) to automatically detect if
+     * long-polling should be used. This is very similar to `experimentalForceLongPolling`,
+     * but only uses long-polling if required.
+     *
+     * This setting will likely be enabled by default in future releases and cannot be
+     * combined with `experimentalForceLongPolling`.
+     *
+     * @webonly
+     */
+    experimentalAutoDetectLongPolling?: boolean;
+
     /**
      * Whether to skip nested properties that are set to `undefined` during
      * object serialization. If set to `true`, these properties are skipped

packages/firestore-types/index.d.ts

@@ -29,6 +29,7 @@ export interface Settings {
   timestampsInSnapshots?: boolean;
   cacheSizeBytes?: number;
   experimentalForceLongPolling?: boolean;
+  experimentalAutoDetectLongPolling?: boolean;
   ignoreUndefinedProperties?: boolean;
   merge?: boolean;
 }

packages/firestore/exp/src/api/database.ts

@@ -121,7 +121,8 @@ export class FirebaseFirestore
       this._persistenceKey,
       settings.host ?? DEFAULT_HOST,
       settings.ssl ?? DEFAULT_SSL,
-      /* forceLongPolling= */ false
+      /* forceLongPolling= */ false,
+      /* forceAutoDetectLongPolling= */ true
     );
     return {
       asyncQueue: this._queue,

packages/firestore/lite/src/api/components.ts

@@ -59,7 +59,8 @@ export function getDatastore(firestore: FirebaseFirestore): Datastore {
       firestore._persistenceKey,
       settings.host ?? DEFAULT_HOST,
       settings.ssl ?? DEFAULT_SSL,
-      /* forceLongPolling= */ false
+      /* forceLongPolling= */ false,
+      /* forceAutoDetectLongPolling= */ true
     );
 
     const connection = newConnection(databaseInfo);

packages/firestore/src/api/database.ts

@@ -106,7 +106,8 @@ import {
   validateOptionNames,
   validatePositiveNumber,
   validateStringEnum,
-  valueDescription
+  valueDescription,
+  validateIsNotUsedTogether
 } from '../util/input_validation';
 import { getLogLevel, logError, LogLevel, setLogLevel } from '../util/log';
 import { AutoId } from '../util/misc';
@@ -146,6 +147,7 @@ const DEFAULT_HOST = 'firestore.googleapis.com';
 const DEFAULT_SSL = true;
 const DEFAULT_TIMESTAMPS_IN_SNAPSHOTS = true;
 const DEFAULT_FORCE_LONG_POLLING = false;
+const DEFAULT_FORCE_AUTO_DETECT_LONG_POLLING = false;
 const DEFAULT_IGNORE_UNDEFINED_PROPERTIES = false;
 
 /**
@@ -191,6 +193,8 @@ class FirestoreSettings {
 
   readonly experimentalForceLongPolling: boolean;
 
+  readonly experimentalAutoDetectLongPolling: boolean;
+
   readonly ignoreUndefinedProperties: boolean;
 
   // Can be a google-auth-library or gapi client.
@@ -221,6 +225,7 @@ class FirestoreSettings {
       'timestampsInSnapshots',
       'cacheSizeBytes',
       'experimentalForceLongPolling',
+      'experimentalAutoDetectLongPolling',
       'ignoreUndefinedProperties'
     ]);
 
@@ -294,6 +299,23 @@ class FirestoreSettings {
     );
     this.experimentalForceLongPolling =
       settings.experimentalForceLongPolling ?? DEFAULT_FORCE_LONG_POLLING;
+
+    validateNamedOptionalType(
+      'settings',
+      'boolean',
+      'experimentalAutoDetectLongPolling',
+      settings.experimentalAutoDetectLongPolling
+    );
+    this.experimentalAutoDetectLongPolling =
+      settings.experimentalAutoDetectLongPolling ??
+      DEFAULT_FORCE_AUTO_DETECT_LONG_POLLING;
+
+    validateIsNotUsedTogether(
+      'experimentalForceLongPolling',
+      settings.experimentalForceLongPolling,
+      'experimentalAutoDetectLongPolling',
+      settings.experimentalAutoDetectLongPolling
+    );
   }
 
   isEqual(other: FirestoreSettings): boolean {
@@ -305,6 +327,8 @@ class FirestoreSettings {
       this.cacheSizeBytes === other.cacheSizeBytes &&
       this.experimentalForceLongPolling ===
         other.experimentalForceLongPolling &&
+      this.experimentalAutoDetectLongPolling ===
+        other.experimentalAutoDetectLongPolling &&
       this.ignoreUndefinedProperties === other.ignoreUndefinedProperties
     );
   }
@@ -552,7 +576,8 @@ export class Firestore implements PublicFirestore, FirebaseService {
       this._persistenceKey,
       this._settings.host,
       this._settings.ssl,
-      this._settings.experimentalForceLongPolling
+      this._settings.experimentalForceLongPolling,
+      this._settings.experimentalAutoDetectLongPolling
     );
   }
 

packages/firestore/src/core/database_info.ts

@@ -29,13 +29,16 @@ export class DatabaseInfo {
    * @param ssl Whether to use SSL when connecting.
    * @param forceLongPolling Whether to use the forceLongPolling option
    * when using WebChannel as the network transport.
+   * @param autoDetectLongPolling Whether to use the detectBufferingProxy
+   * option when using WebChannel as the network transport.
    */
   constructor(
     readonly databaseId: DatabaseId,
     readonly persistenceKey: string,
     readonly host: string,
     readonly ssl: boolean,
-    readonly forceLongPolling: boolean
+    readonly forceLongPolling: boolean,
+    readonly autoDetectLongPolling: boolean
   ) {}
 }
 

packages/firestore/src/platform/browser/webchannel_connection.ts

@@ -57,10 +57,12 @@ const XHR_TIMEOUT_SECS = 15;
 
 export class WebChannelConnection extends RestConnection {
   private readonly forceLongPolling: boolean;
+  private readonly autoDetectLongPolling: boolean;
 
   constructor(info: DatabaseInfo) {
     super(info);
     this.forceLongPolling = info.forceLongPolling;
+    this.autoDetectLongPolling = info.autoDetectLongPolling;
   }
 
   protected performRPCRequest<Req, Resp>(
@@ -183,7 +185,8 @@ export class WebChannelConnection extends RestConnection {
         // timeouts to kick in if the request isn't working.
         forwardChannelRequestTimeoutMs: 10 * 60 * 1000
       },
-      forceLongPolling: this.forceLongPolling
+      forceLongPolling: this.forceLongPolling,
+      detectBufferingProxy: this.autoDetectLongPolling
     };
 
     this.modifyHeadersForRequest(request.initMessageHeaders!, token);

packages/firestore/src/util/input_validation.ts

@@ -199,6 +199,23 @@ export function validateNamedOptionalType(
   }
 }
 
+/**
+ * Validates that two boolean options are not set at the same time.
+ */
+export function validateIsNotUsedTogether(
+  optionName1: string,
+  argument1: boolean | undefined,
+  optionName2: string,
+  argument2: boolean | undefined
+): void {
+  if (argument1 === true && argument2 === true) {
+    throw new FirestoreError(
+      Code.INVALID_ARGUMENT,
+      `${optionName1} and ${optionName2} cannot be used together.`
+    );
+  }
+}
+
 export function validateArrayElements<T>(
   functionName: string,
   optionName: string,

packages/firestore/test/integration/api/database.test.ts

@@ -26,12 +26,13 @@ import * as firebaseExport from '../util/firebase_export';
 import {
   apiDescribe,
   withTestCollection,
+  withTestDbsSettings,
   withTestDb,
   withTestDbs,
   withTestDoc,
   withTestDocAndInitialData
 } from '../util/helpers';
-import { DEFAULT_SETTINGS } from '../util/settings';
+import { DEFAULT_SETTINGS, DEFAULT_PROJECT_ID } from '../util/settings';
 
 use(chaiAsPromised);
 
@@ -1600,4 +1601,30 @@ apiDescribe('Database', (persistence: boolean) => {
       });
     });
   });
+
+  it('can set and get data with auto detect long polling enabled', () => {
+    const settings = {
+      ...DEFAULT_SETTINGS,
+      experimentalAutoDetectLongPolling: true
+    };
+
+    return withTestDbsSettings(
+      persistence,
+      DEFAULT_PROJECT_ID,
+      settings,
+      1,
+      async ([db]) => {
+        const data = { name: 'Rafi', email: '[email protected]' };
+        const doc = await db.collection('users').doc();
+
+        return doc
+          .set(data)
+          .then(() => doc.get())
+          .then(snapshot => {
+            expect(snapshot.exists).to.be.ok;
+            expect(snapshot.data()).to.deep.equal(data);
+          });
+      }
+    );
+  });
 });

packages/firestore/test/integration/util/internal_helpers.ts

@@ -44,7 +44,8 @@ export function getDefaultDatabaseInfo(): DatabaseInfo {
     'persistenceKey',
     DEFAULT_SETTINGS.host!,
     !!DEFAULT_SETTINGS.ssl,
-    !!DEFAULT_SETTINGS.experimentalForceLongPolling
+    !!DEFAULT_SETTINGS.experimentalForceLongPolling,
+    !!DEFAULT_SETTINGS.experimentalAutoDetectLongPolling
   );
 }
 

packages/firestore/test/unit/api/database.test.ts

@@ -170,6 +170,19 @@ describe('Settings', () => {
     );
   });
 
+  it('can not use mutually exclusive settings together', () => {
+    // Use a new instance of Firestore in order to configure settings.
+    const firestoreClient = newTestFirestore();
+    expect(
+      firestoreClient.settings.bind(firestoreClient.settings, {
+        experimentalForceLongPolling: true,
+        experimentalAutoDetectLongPolling: true
+      })
+    ).to.throw(
+      `experimentalForceLongPolling and experimentalAutoDetectLongPolling cannot be used together.`
+    );
+  });
+
   it('can merge settings', () => {
     // Use a new instance of Firestore in order to configure settings.
     const firestoreClient = newTestFirestore();

packages/firestore/test/unit/remote/rest_connection.test.ts

@@ -60,7 +60,8 @@ describe('RestConnection', () => {
     'persistenceKey',
     'example.com',
     /*ssl=*/ false,
-    /*forceLongPolling=*/ false
+    /*forceLongPolling=*/ false,
+    /*autoDetectLongPolling=*/ false
   );
   const connection = new TestRestConnection(testDatabaseInfo);
 

packages/firestore/test/unit/specs/spec_test_runner.ts

@@ -241,7 +241,8 @@ abstract class TestRunner {
       TEST_PERSISTENCE_KEY,
       'host',
       /*ssl=*/ false,
-      /*forceLongPolling=*/ false
+      /*forceLongPolling=*/ false,
+      /*autoDetectLongPolling=*/ false
     );
 
     // TODO(mrschmidt): During client startup in `firestore_client`, we block

packages/webchannel-wrapper/src/index.d.ts

@@ -89,6 +89,7 @@ export interface WebChannelOptions {
   httpSessionIdParam?: string;
   httpHeadersOverwriteParam?: string;
   forceLongPolling?: boolean;
+  detectBufferingProxy?: boolean;
   fastHandshake?: boolean;
   disableRedac?: boolean;
   clientProfile?: string;