From f0623762965dbb2ea0ed255962ccc038e1dd3ec5 Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Wed, 19 Apr 2023 21:53:35 -0400 Subject: [PATCH 01/15] set experimentalAutoDetectLongPolling default value to true --- packages/firestore/src/lite-api/settings.ts | 16 ++-- .../firestore/test/unit/api/database.test.ts | 80 +++++++++++++++++++ 2 files changed, 91 insertions(+), 5 deletions(-) diff --git a/packages/firestore/src/lite-api/settings.ts b/packages/firestore/src/lite-api/settings.ts index de7de2b79bb..f014701b733 100644 --- a/packages/firestore/src/lite-api/settings.ts +++ b/packages/firestore/src/lite-api/settings.ts @@ -123,17 +123,23 @@ export class FirestoreSettingsImpl { } } - this.experimentalForceLongPolling = !!settings.experimentalForceLongPolling; - this.experimentalAutoDetectLongPolling = - !!settings.experimentalAutoDetectLongPolling; - this.useFetchStreams = !!settings.useFetchStreams; - validateIsNotUsedTogether( 'experimentalForceLongPolling', settings.experimentalForceLongPolling, 'experimentalAutoDetectLongPolling', settings.experimentalAutoDetectLongPolling ); + + if (!settings.experimentalForceLongPolling) { + this.experimentalForceLongPolling = false; + this.experimentalAutoDetectLongPolling = + settings.experimentalAutoDetectLongPolling ?? true; + } else { + this.experimentalForceLongPolling = true; + this.experimentalAutoDetectLongPolling = false; + } + + this.useFetchStreams = !!settings.useFetchStreams; } isEqual(other: FirestoreSettingsImpl): boolean { diff --git a/packages/firestore/test/unit/api/database.test.ts b/packages/firestore/test/unit/api/database.test.ts index edc1a520f07..5ffae78c564 100644 --- a/packages/firestore/test/unit/api/database.test.ts +++ b/packages/firestore/test/unit/api/database.test.ts @@ -279,6 +279,86 @@ describe('Settings', () => { ); }); + it('long polling should be in auto-detect mode by default', () => { + // Use a new instance of Firestore in order to configure settings. + const db = newTestFirestore(); + expect(db._getSettings().experimentalAutoDetectLongPolling).to.be.true; + expect(db._getSettings().experimentalForceLongPolling).to.be.false; + }); + + it('long polling should be in force mode if force=true', () => { + // Use a new instance of Firestore in order to configure settings. + const db = newTestFirestore(); + db._setSettings({ + experimentalForceLongPolling: true + }); + expect(db._getSettings().experimentalAutoDetectLongPolling).to.be.false; + expect(db._getSettings().experimentalForceLongPolling).to.be.true; + }); + + it('long polling should be in auto-detect mode if autoDetect=true', () => { + // Use a new instance of Firestore in order to configure settings. + const db = newTestFirestore(); + db._setSettings({ + experimentalAutoDetectLongPolling: true + }); + expect(db._getSettings().experimentalAutoDetectLongPolling).to.be.true; + expect(db._getSettings().experimentalForceLongPolling).to.be.false; + }); + + it('long polling should be in auto-detect mode if force=false', () => { + // Use a new instance of Firestore in order to configure settings. + const db = newTestFirestore(); + db._setSettings({ + experimentalForceLongPolling: false + }); + expect(db._getSettings().experimentalAutoDetectLongPolling).to.be.true; + expect(db._getSettings().experimentalForceLongPolling).to.be.false; + }); + + it('long polling should be disabled if autoDetect=false', () => { + // Use a new instance of Firestore in order to configure settings. + const db = newTestFirestore(); + db._setSettings({ + experimentalAutoDetectLongPolling: false + }); + expect(db._getSettings().experimentalAutoDetectLongPolling).to.be.false; + expect(db._getSettings().experimentalForceLongPolling).to.be.false; + }); + + it('long polling should be in auto-detect mode if autoDetect=true and force=false', () => { + // Use a new instance of Firestore in order to configure settings. + const db = newTestFirestore(); + db._setSettings({ + experimentalAutoDetectLongPolling: true, + experimentalForceLongPolling: false + }); + expect(db._getSettings().experimentalAutoDetectLongPolling).to.be.true; + expect(db._getSettings().experimentalForceLongPolling).to.be.false; + }); + + it('long polling should be in force mode if autoDetect=false and force=true', () => { + // Use a new instance of Firestore in order to configure settings. + const db = newTestFirestore(); + db._setSettings({ + experimentalAutoDetectLongPolling: false, + experimentalForceLongPolling: true + }); + expect(db._getSettings().experimentalAutoDetectLongPolling).to.be.false; + expect(db._getSettings().experimentalForceLongPolling).to.be.true; + }); + + it('long polling should be disabled if autoDetect=false and force=false', () => { + // Use a new instance of Firestore in order to configure settings. + const db = newTestFirestore(); + db._setSettings({ + experimentalAutoDetectLongPolling: false, + experimentalForceLongPolling: false + }); + expect(db._getSettings().experimentalAutoDetectLongPolling).to.be.false; + expect(db._getSettings().experimentalForceLongPolling).to.be.false; + }); + it('gets settings from useEmulator', () => { // Use a new instance of Firestore in order to configure settings. const db = newTestFirestore(); From 65d5e9cb80975830471b11433f2d84e45b194191 Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Wed, 19 Apr 2023 22:08:44 -0400 Subject: [PATCH 02/15] changeset --- .changeset/long-lemons-change.md | 6 ++++++ 1 file changed, 6 insertions(+) create mode 100644 .changeset/long-lemons-change.md diff --git a/.changeset/long-lemons-change.md b/.changeset/long-lemons-change.md new file mode 100644 index 00000000000..b436afacb47 --- /dev/null +++ b/.changeset/long-lemons-change.md @@ -0,0 +1,6 @@ +--- +'@firebase/firestore': minor +'firebase': minor +--- + +Enabled "auto-detect long polling" networking mode by default. It can be explicitly disabled by setting `FirestoreSettings.experimentalForceLongPolling` to `false`. From 959c0e7c1a2d5d154682cf2985624edf2012f95a Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Wed, 19 Apr 2023 22:25:38 -0400 Subject: [PATCH 03/15] Firestore: database.test.ts: add unit tests for long polling settings interpretation. --- .../firestore/test/unit/api/database.test.ts | 80 +++++++++++++++++++ 1 file changed, 80 insertions(+) diff --git a/packages/firestore/test/unit/api/database.test.ts b/packages/firestore/test/unit/api/database.test.ts index edc1a520f07..38f4c4eedd0 100644 --- a/packages/firestore/test/unit/api/database.test.ts +++ b/packages/firestore/test/unit/api/database.test.ts @@ -279,6 +279,86 @@ describe('Settings', () => { ); }); + it('long polling should be disabled by default', () => { + // Use a new instance of Firestore in order to configure settings. + const db = newTestFirestore(); + expect(db._getSettings().experimentalAutoDetectLongPolling).to.be.false; + expect(db._getSettings().experimentalForceLongPolling).to.be.false; + }); + + it('long polling should be in force mode if force=true', () => { + // Use a new instance of Firestore in order to configure settings. + const db = newTestFirestore(); + db._setSettings({ + experimentalForceLongPolling: true + }); + expect(db._getSettings().experimentalAutoDetectLongPolling).to.be.false; + expect(db._getSettings().experimentalForceLongPolling).to.be.true; + }); + + it('long polling should be in auto-detect mode if autoDetect=true', () => { + // Use a new instance of Firestore in order to configure settings. + const db = newTestFirestore(); + db._setSettings({ + experimentalAutoDetectLongPolling: true + }); + expect(db._getSettings().experimentalAutoDetectLongPolling).to.be.true; + expect(db._getSettings().experimentalForceLongPolling).to.be.false; + }); + + it('long polling should be disabled if force=false', () => { + // Use a new instance of Firestore in order to configure settings. + const db = newTestFirestore(); + db._setSettings({ + experimentalForceLongPolling: false + }); + expect(db._getSettings().experimentalAutoDetectLongPolling).to.be.false; + expect(db._getSettings().experimentalForceLongPolling).to.be.false; + }); + + it('long polling should be disabled if autoDetect=false', () => { + // Use a new instance of Firestore in order to configure settings. + const db = newTestFirestore(); + db._setSettings({ + experimentalAutoDetectLongPolling: false + }); + expect(db._getSettings().experimentalAutoDetectLongPolling).to.be.false; + expect(db._getSettings().experimentalForceLongPolling).to.be.false; + }); + + it('long polling should be in auto-detect mode if autoDetect=true and force=false', () => { + // Use a new instance of Firestore in order to configure settings. + const db = newTestFirestore(); + db._setSettings({ + experimentalAutoDetectLongPolling: true, + experimentalForceLongPolling: false + }); + expect(db._getSettings().experimentalAutoDetectLongPolling).to.be.true; + expect(db._getSettings().experimentalForceLongPolling).to.be.false; + }); + + it('long polling should be in force mode if autoDetect=false and force=true', () => { + // Use a new instance of Firestore in order to configure settings. + const db = newTestFirestore(); + db._setSettings({ + experimentalAutoDetectLongPolling: false, + experimentalForceLongPolling: true + }); + expect(db._getSettings().experimentalAutoDetectLongPolling).to.be.false; + expect(db._getSettings().experimentalForceLongPolling).to.be.true; + }); + + it('long polling should be disabled if autoDetect=false and force=false', () => { + // Use a new instance of Firestore in order to configure settings. + const db = newTestFirestore(); + db._setSettings({ + experimentalAutoDetectLongPolling: false, + experimentalForceLongPolling: false + }); + expect(db._getSettings().experimentalAutoDetectLongPolling).to.be.false; + expect(db._getSettings().experimentalForceLongPolling).to.be.false; + }); + it('gets settings from useEmulator', () => { // Use a new instance of Firestore in order to configure settings. const db = newTestFirestore(); From ac752d1b35d4359f40fc5ae9fbd9122b7ee60410 Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Wed, 19 Apr 2023 22:34:53 -0400 Subject: [PATCH 04/15] Update documentation of experimentalAutoDetectLongPolling to document that its default value is now true --- packages/firestore/src/api/settings.ts | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/packages/firestore/src/api/settings.ts b/packages/firestore/src/api/settings.ts index 8675b28ca96..23d08d6dab3 100644 --- a/packages/firestore/src/api/settings.ts +++ b/packages/firestore/src/api/settings.ts @@ -88,8 +88,11 @@ export interface FirestoreSettings extends LiteSettings { * 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`. + * After having had a default value of `false` since its inception 4 years + * ago, the default value of this setting was changed in the most recent + * release to `true`. That is, auto-detection of long polling is now enabled + * by default. To disable it, set this setting to `false`, and please open a + * GitHub issue to share your problems with long-polling auto-detection. */ experimentalAutoDetectLongPolling?: boolean; } From 82189100a312b65a2dd0795b1b69c33d8b64cbc1 Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Wed, 19 Apr 2023 22:36:19 -0400 Subject: [PATCH 05/15] reword changeset --- .changeset/long-lemons-change.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.changeset/long-lemons-change.md b/.changeset/long-lemons-change.md index b436afacb47..da79973d1eb 100644 --- a/.changeset/long-lemons-change.md +++ b/.changeset/long-lemons-change.md @@ -3,4 +3,4 @@ 'firebase': minor --- -Enabled "auto-detect long polling" networking mode by default. It can be explicitly disabled by setting `FirestoreSettings.experimentalForceLongPolling` to `false`. +Enabled long-polling networking mode auto detection by default. It can be explicitly disabled by setting `FirestoreSettings.experimentalForceLongPolling` to `false`. From 09d6f312049f08dc4dd12881969f40da538dbbe8 Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Wed, 19 Apr 2023 22:58:56 -0400 Subject: [PATCH 06/15] yarn docgen devsite --- docs-devsite/firestore_.firestoresettings.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/docs-devsite/firestore_.firestoresettings.md b/docs-devsite/firestore_.firestoresettings.md index e4d4846ee64..e13847de556 100644 --- a/docs-devsite/firestore_.firestoresettings.md +++ b/docs-devsite/firestore_.firestoresettings.md @@ -23,7 +23,7 @@ export declare interface FirestoreSettings | Property | Type | Description | | --- | --- | --- | | [cacheSizeBytes](./firestore_.firestoresettings.md#firestoresettingscachesizebytes) | number | NOTE: This field will be deprecated in a future major release. Use cache field instead to specify cache size, and other cache configurations.An approximate cache size threshold for the on-disk data. If the cache grows beyond this size, Firestore will start removing data that hasn't been recently used. The size is not a guarantee that the cache will stay below that size, only that if the cache exceeds the given size, cleanup will be attempted.The default value is 40 MB. The threshold must be set to at least 1 MB, and can be set to CACHE_SIZE_UNLIMITED to disable garbage collection. | -| [experimentalAutoDetectLongPolling](./firestore_.firestoresettings.md#firestoresettingsexperimentalautodetectlongpolling) | 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. | +| [experimentalAutoDetectLongPolling](./firestore_.firestoresettings.md#firestoresettingsexperimentalautodetectlongpolling) | 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.After having had a default value of false since its inception 4 years ago, the default value of this setting was changed in the most recent release to true. That is, auto-detection of long polling is now enabled by default. To disable it, set this setting to false, and please open a GitHub issue to share your problems with long-polling auto-detection. | | [experimentalForceLongPolling](./firestore_.firestoresettings.md#firestoresettingsexperimentalforcelongpolling) | boolean | Forces the SDK’s underlying network transport (WebChannel) to use long-polling. Each response from the backend will be closed immediately after the backend sends data (by default responses are kept open in case the backend has more data to send). This avoids incompatibility issues with certain proxies, antivirus software, etc. that incorrectly buffer traffic indefinitely. Use of this option will cause some performance degradation though.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. | | [host](./firestore_.firestoresettings.md#firestoresettingshost) | string | The hostname to connect to. | | [ignoreUndefinedProperties](./firestore_.firestoresettings.md#firestoresettingsignoreundefinedproperties) | boolean | Whether to skip nested properties that are set to undefined during object serialization. If set to true, these properties are skipped and not written to Firestore. If set to false or omitted, the SDK throws an exception when it encounters properties of type undefined. | @@ -48,7 +48,7 @@ cacheSizeBytes?: number; 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`. +After having had a default value of `false` since its inception 4 years ago, the default value of this setting was changed in the most recent release to `true`. That is, auto-detection of long polling is now enabled by default. To disable it, set this setting to `false`, and please open a GitHub issue to share your problems with long-polling auto-detection. Signature: From eb854dc34357796d4ca10ca28a0da8f2aa6e8d20 Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Thu, 20 Apr 2023 09:57:21 -0400 Subject: [PATCH 07/15] Firestore: settings.ts: very minor refactor of long-polling logic. --- packages/firestore/src/lite-api/settings.ts | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/packages/firestore/src/lite-api/settings.ts b/packages/firestore/src/lite-api/settings.ts index de7de2b79bb..fb6848ee5bb 100644 --- a/packages/firestore/src/lite-api/settings.ts +++ b/packages/firestore/src/lite-api/settings.ts @@ -123,17 +123,23 @@ export class FirestoreSettingsImpl { } } - this.experimentalForceLongPolling = !!settings.experimentalForceLongPolling; - this.experimentalAutoDetectLongPolling = - !!settings.experimentalAutoDetectLongPolling; - this.useFetchStreams = !!settings.useFetchStreams; - validateIsNotUsedTogether( 'experimentalForceLongPolling', settings.experimentalForceLongPolling, 'experimentalAutoDetectLongPolling', settings.experimentalAutoDetectLongPolling ); + + if (!settings.experimentalForceLongPolling) { + this.experimentalForceLongPolling = false; + this.experimentalAutoDetectLongPolling = + settings.experimentalAutoDetectLongPolling ?? false; + } else { + this.experimentalForceLongPolling = true; + this.experimentalAutoDetectLongPolling = false; + } + + this.useFetchStreams = !!settings.useFetchStreams; } isEqual(other: FirestoreSettingsImpl): boolean { From ef8665f8a264fc8ac15f0757fc27aa7ccd7252c5 Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Thu, 20 Apr 2023 10:50:56 -0400 Subject: [PATCH 08/15] invert negative 'if' statement and ensure that experimentalAutoDetectLongPolling is boolean --- packages/firestore/src/lite-api/settings.ts | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/packages/firestore/src/lite-api/settings.ts b/packages/firestore/src/lite-api/settings.ts index fb6848ee5bb..a74a337e99e 100644 --- a/packages/firestore/src/lite-api/settings.ts +++ b/packages/firestore/src/lite-api/settings.ts @@ -130,13 +130,17 @@ export class FirestoreSettingsImpl { settings.experimentalAutoDetectLongPolling ); - if (!settings.experimentalForceLongPolling) { - this.experimentalForceLongPolling = false; - this.experimentalAutoDetectLongPolling = - settings.experimentalAutoDetectLongPolling ?? false; - } else { + if (settings.experimentalForceLongPolling) { this.experimentalForceLongPolling = true; this.experimentalAutoDetectLongPolling = false; + } else { + this.experimentalForceLongPolling = false; + if (settings.experimentalAutoDetectLongPolling === undefined) { + this.experimentalAutoDetectLongPolling = false; + } else { + this.experimentalAutoDetectLongPolling = + settings.experimentalAutoDetectLongPolling; + } } this.useFetchStreams = !!settings.useFetchStreams; From 8e84d43a5759c50470f83d3898843e78690101d0 Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Thu, 20 Apr 2023 12:22:35 -0400 Subject: [PATCH 09/15] add DEFAULT_AUTO_DETECT_LONG_POLLING and coerce to boolean --- packages/firestore/src/lite-api/settings.ts | 21 ++++++----- .../firestore/test/unit/api/database.test.ts | 36 +++++++++++++++++++ 2 files changed, 48 insertions(+), 9 deletions(-) diff --git a/packages/firestore/src/lite-api/settings.ts b/packages/firestore/src/lite-api/settings.ts index a74a337e99e..ae34a732019 100644 --- a/packages/firestore/src/lite-api/settings.ts +++ b/packages/firestore/src/lite-api/settings.ts @@ -29,6 +29,8 @@ import { validateIsNotUsedTogether } from '../util/input_validation'; export const DEFAULT_HOST = 'firestore.googleapis.com'; export const DEFAULT_SSL = true; +const DEFAULT_AUTO_DETECT_LONG_POLLING = false; + /** * Specifies custom configurations for your Cloud Firestore instance. * You must set these before invoking any other methods. @@ -130,17 +132,18 @@ export class FirestoreSettingsImpl { settings.experimentalAutoDetectLongPolling ); - if (settings.experimentalForceLongPolling) { - this.experimentalForceLongPolling = true; + this.experimentalForceLongPolling = !!settings.experimentalForceLongPolling; + + if (this.experimentalForceLongPolling) { this.experimentalAutoDetectLongPolling = false; + } else if (settings.experimentalAutoDetectLongPolling !== undefined) { + // For backwards compatibility, coerce the value to boolean even though + // the TypeScript compiler has narrowed the type to boolean already. + // noinspection PointlessBooleanExpressionJS + this.experimentalAutoDetectLongPolling = + !!settings.experimentalAutoDetectLongPolling; } else { - this.experimentalForceLongPolling = false; - if (settings.experimentalAutoDetectLongPolling === undefined) { - this.experimentalAutoDetectLongPolling = false; - } else { - this.experimentalAutoDetectLongPolling = - settings.experimentalAutoDetectLongPolling; - } + this.experimentalAutoDetectLongPolling = DEFAULT_AUTO_DETECT_LONG_POLLING; } this.useFetchStreams = !!settings.useFetchStreams; diff --git a/packages/firestore/test/unit/api/database.test.ts b/packages/firestore/test/unit/api/database.test.ts index 38f4c4eedd0..93e47b360f7 100644 --- a/packages/firestore/test/unit/api/database.test.ts +++ b/packages/firestore/test/unit/api/database.test.ts @@ -359,6 +359,42 @@ describe('Settings', () => { expect(db._getSettings().experimentalForceLongPolling).to.be.false; }); + it('long polling autoDetect=[something truthy] is corced to true', () => { + // Use a new instance of Firestore in order to configure settings. + const db = newTestFirestore(); + db._setSettings({ + experimentalAutoDetectLongPolling: 1 as any, + }); + expect(db._getSettings().experimentalAutoDetectLongPolling).to.be.true; + }); + + it('long polling autoDetect=[something falsy] is corced to false', () => { + // Use a new instance of Firestore in order to configure settings. + const db = newTestFirestore(); + db._setSettings({ + experimentalAutoDetectLongPolling: 0 as any, + }); + expect(db._getSettings().experimentalAutoDetectLongPolling).to.be.false; + }); + + it('long polling force=[something truthy] is corced to true', () => { + // Use a new instance of Firestore in order to configure settings. + const db = newTestFirestore(); + db._setSettings({ + experimentalForceLongPolling: "I am truthy" as any, + }); + expect(db._getSettings().experimentalForceLongPolling).to.be.true; + }); + + it('long polling force=[something falsy] is corced to false', () => { + // Use a new instance of Firestore in order to configure settings. + const db = newTestFirestore(); + db._setSettings({ + experimentalForceLongPolling: NaN as any, + }); + expect(db._getSettings().experimentalForceLongPolling).to.be.false; + }); + it('gets settings from useEmulator', () => { // Use a new instance of Firestore in order to configure settings. const db = newTestFirestore(); From 3e2be998df812ddc12f68f091bda3441e54c5839 Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Thu, 20 Apr 2023 12:24:10 -0400 Subject: [PATCH 10/15] Firestore: database.test.ts: add tests for coercing experimentalForceLongPolling and experimentalAutoDetectLongPolling to boolean --- .../firestore/test/unit/api/database.test.ts | 36 +++++++++++++++++++ 1 file changed, 36 insertions(+) diff --git a/packages/firestore/test/unit/api/database.test.ts b/packages/firestore/test/unit/api/database.test.ts index 38f4c4eedd0..93e47b360f7 100644 --- a/packages/firestore/test/unit/api/database.test.ts +++ b/packages/firestore/test/unit/api/database.test.ts @@ -359,6 +359,42 @@ describe('Settings', () => { expect(db._getSettings().experimentalForceLongPolling).to.be.false; }); + it('long polling autoDetect=[something truthy] is corced to true', () => { + // Use a new instance of Firestore in order to configure settings. + const db = newTestFirestore(); + db._setSettings({ + experimentalAutoDetectLongPolling: 1 as any, + }); + expect(db._getSettings().experimentalAutoDetectLongPolling).to.be.true; + }); + + it('long polling autoDetect=[something falsy] is corced to false', () => { + // Use a new instance of Firestore in order to configure settings. + const db = newTestFirestore(); + db._setSettings({ + experimentalAutoDetectLongPolling: 0 as any, + }); + expect(db._getSettings().experimentalAutoDetectLongPolling).to.be.false; + }); + + it('long polling force=[something truthy] is corced to true', () => { + // Use a new instance of Firestore in order to configure settings. + const db = newTestFirestore(); + db._setSettings({ + experimentalForceLongPolling: "I am truthy" as any, + }); + expect(db._getSettings().experimentalForceLongPolling).to.be.true; + }); + + it('long polling force=[something falsy] is corced to false', () => { + // Use a new instance of Firestore in order to configure settings. + const db = newTestFirestore(); + db._setSettings({ + experimentalForceLongPolling: NaN as any, + }); + expect(db._getSettings().experimentalForceLongPolling).to.be.false; + }); + it('gets settings from useEmulator', () => { // Use a new instance of Firestore in order to configure settings. const db = newTestFirestore(); From d8fccbea6de14057309948cdbbd2a77769c919a2 Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Thu, 20 Apr 2023 12:28:56 -0400 Subject: [PATCH 11/15] yarn prettier --- packages/firestore/test/unit/api/database.test.ts | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/packages/firestore/test/unit/api/database.test.ts b/packages/firestore/test/unit/api/database.test.ts index 93e47b360f7..5f7ffbf419e 100644 --- a/packages/firestore/test/unit/api/database.test.ts +++ b/packages/firestore/test/unit/api/database.test.ts @@ -363,7 +363,7 @@ describe('Settings', () => { // Use a new instance of Firestore in order to configure settings. const db = newTestFirestore(); db._setSettings({ - experimentalAutoDetectLongPolling: 1 as any, + experimentalAutoDetectLongPolling: 1 as any }); expect(db._getSettings().experimentalAutoDetectLongPolling).to.be.true; }); @@ -372,7 +372,7 @@ describe('Settings', () => { // Use a new instance of Firestore in order to configure settings. const db = newTestFirestore(); db._setSettings({ - experimentalAutoDetectLongPolling: 0 as any, + experimentalAutoDetectLongPolling: 0 as any }); expect(db._getSettings().experimentalAutoDetectLongPolling).to.be.false; }); @@ -381,7 +381,7 @@ describe('Settings', () => { // Use a new instance of Firestore in order to configure settings. const db = newTestFirestore(); db._setSettings({ - experimentalForceLongPolling: "I am truthy" as any, + experimentalForceLongPolling: 'I am truthy' as any }); expect(db._getSettings().experimentalForceLongPolling).to.be.true; }); @@ -390,7 +390,7 @@ describe('Settings', () => { // Use a new instance of Firestore in order to configure settings. const db = newTestFirestore(); db._setSettings({ - experimentalForceLongPolling: NaN as any, + experimentalForceLongPolling: NaN as any }); expect(db._getSettings().experimentalForceLongPolling).to.be.false; }); From 74278583e313401753b6ed6ffc75243cc99ea432 Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Thu, 20 Apr 2023 12:35:03 -0400 Subject: [PATCH 12/15] tweak unit test descriptions --- packages/firestore/test/unit/api/database.test.ts | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/packages/firestore/test/unit/api/database.test.ts b/packages/firestore/test/unit/api/database.test.ts index 5f7ffbf419e..8d6a08977e4 100644 --- a/packages/firestore/test/unit/api/database.test.ts +++ b/packages/firestore/test/unit/api/database.test.ts @@ -359,7 +359,7 @@ describe('Settings', () => { expect(db._getSettings().experimentalForceLongPolling).to.be.false; }); - it('long polling autoDetect=[something truthy] is corced to true', () => { + it('long polling autoDetect=[something truthy] should be coerced to true', () => { // Use a new instance of Firestore in order to configure settings. const db = newTestFirestore(); db._setSettings({ @@ -368,7 +368,7 @@ describe('Settings', () => { expect(db._getSettings().experimentalAutoDetectLongPolling).to.be.true; }); - it('long polling autoDetect=[something falsy] is corced to false', () => { + it('long polling autoDetect=[something falsy] should be coerced to false', () => { // Use a new instance of Firestore in order to configure settings. const db = newTestFirestore(); db._setSettings({ @@ -377,7 +377,7 @@ describe('Settings', () => { expect(db._getSettings().experimentalAutoDetectLongPolling).to.be.false; }); - it('long polling force=[something truthy] is corced to true', () => { + it('long polling force=[something truthy] should be coerced to true', () => { // Use a new instance of Firestore in order to configure settings. const db = newTestFirestore(); db._setSettings({ @@ -386,7 +386,7 @@ describe('Settings', () => { expect(db._getSettings().experimentalForceLongPolling).to.be.true; }); - it('long polling force=[something falsy] is corced to false', () => { + it('long polling force=[something falsy] should be coerced to false', () => { // Use a new instance of Firestore in order to configure settings. const db = newTestFirestore(); db._setSettings({ From 81a61b465af7d63569dbc20e6e83e46d9d751450 Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Thu, 20 Apr 2023 12:59:32 -0400 Subject: [PATCH 13/15] add `// eslint-disable-next-line @typescript-eslint/no-explicit-any` lines to make lint happy --- packages/firestore/test/unit/api/database.test.ts | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/packages/firestore/test/unit/api/database.test.ts b/packages/firestore/test/unit/api/database.test.ts index 8d6a08977e4..7668c3d0679 100644 --- a/packages/firestore/test/unit/api/database.test.ts +++ b/packages/firestore/test/unit/api/database.test.ts @@ -363,6 +363,7 @@ describe('Settings', () => { // Use a new instance of Firestore in order to configure settings. const db = newTestFirestore(); db._setSettings({ + // eslint-disable-next-line @typescript-eslint/no-explicit-any experimentalAutoDetectLongPolling: 1 as any }); expect(db._getSettings().experimentalAutoDetectLongPolling).to.be.true; @@ -372,6 +373,7 @@ describe('Settings', () => { // Use a new instance of Firestore in order to configure settings. const db = newTestFirestore(); db._setSettings({ + // eslint-disable-next-line @typescript-eslint/no-explicit-any experimentalAutoDetectLongPolling: 0 as any }); expect(db._getSettings().experimentalAutoDetectLongPolling).to.be.false; @@ -381,6 +383,7 @@ describe('Settings', () => { // Use a new instance of Firestore in order to configure settings. const db = newTestFirestore(); db._setSettings({ + // eslint-disable-next-line @typescript-eslint/no-explicit-any experimentalForceLongPolling: 'I am truthy' as any }); expect(db._getSettings().experimentalForceLongPolling).to.be.true; @@ -390,6 +393,7 @@ describe('Settings', () => { // Use a new instance of Firestore in order to configure settings. const db = newTestFirestore(); db._setSettings({ + // eslint-disable-next-line @typescript-eslint/no-explicit-any experimentalForceLongPolling: NaN as any }); expect(db._getSettings().experimentalForceLongPolling).to.be.false; From 17519c3683d89b340437f1fd5861b67da83d271e Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Thu, 20 Apr 2023 13:41:34 -0400 Subject: [PATCH 14/15] invert a negative 'if' statement --- packages/firestore/src/lite-api/settings.ts | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/packages/firestore/src/lite-api/settings.ts b/packages/firestore/src/lite-api/settings.ts index ae34a732019..e9b4e7658c5 100644 --- a/packages/firestore/src/lite-api/settings.ts +++ b/packages/firestore/src/lite-api/settings.ts @@ -136,14 +136,14 @@ export class FirestoreSettingsImpl { if (this.experimentalForceLongPolling) { this.experimentalAutoDetectLongPolling = false; - } else if (settings.experimentalAutoDetectLongPolling !== undefined) { + } else if (settings.experimentalAutoDetectLongPolling === undefined) { + this.experimentalAutoDetectLongPolling = DEFAULT_AUTO_DETECT_LONG_POLLING; + } else { // For backwards compatibility, coerce the value to boolean even though // the TypeScript compiler has narrowed the type to boolean already. // noinspection PointlessBooleanExpressionJS this.experimentalAutoDetectLongPolling = !!settings.experimentalAutoDetectLongPolling; - } else { - this.experimentalAutoDetectLongPolling = DEFAULT_AUTO_DETECT_LONG_POLLING; } this.useFetchStreams = !!settings.useFetchStreams; From f166d1d910841bf3beb9d5c9a8cf6c895c29b701 Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Fri, 21 Apr 2023 17:52:19 +0000 Subject: [PATCH 15/15] tweak api docs for experimentalAutoDetectLongPolling, as suggested in code review --- docs-devsite/firestore_.firestoresettings.md | 4 ++-- packages/firestore/src/api/settings.ts | 10 +++++----- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/docs-devsite/firestore_.firestoresettings.md b/docs-devsite/firestore_.firestoresettings.md index e13847de556..66ae4b95acd 100644 --- a/docs-devsite/firestore_.firestoresettings.md +++ b/docs-devsite/firestore_.firestoresettings.md @@ -23,7 +23,7 @@ export declare interface FirestoreSettings | Property | Type | Description | | --- | --- | --- | | [cacheSizeBytes](./firestore_.firestoresettings.md#firestoresettingscachesizebytes) | number | NOTE: This field will be deprecated in a future major release. Use cache field instead to specify cache size, and other cache configurations.An approximate cache size threshold for the on-disk data. If the cache grows beyond this size, Firestore will start removing data that hasn't been recently used. The size is not a guarantee that the cache will stay below that size, only that if the cache exceeds the given size, cleanup will be attempted.The default value is 40 MB. The threshold must be set to at least 1 MB, and can be set to CACHE_SIZE_UNLIMITED to disable garbage collection. | -| [experimentalAutoDetectLongPolling](./firestore_.firestoresettings.md#firestoresettingsexperimentalautodetectlongpolling) | 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.After having had a default value of false since its inception 4 years ago, the default value of this setting was changed in the most recent release to true. That is, auto-detection of long polling is now enabled by default. To disable it, set this setting to false, and please open a GitHub issue to share your problems with long-polling auto-detection. | +| [experimentalAutoDetectLongPolling](./firestore_.firestoresettings.md#firestoresettingsexperimentalautodetectlongpolling) | 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.After having had a default value of false since its inception in 2019, the default value of this setting was changed in mid-2023 to true. That is, auto-detection of long polling is now enabled by default. To disable it, set this setting to false, and please open a GitHub issue to share the problems that motivated you disabling long-polling auto-detection. | | [experimentalForceLongPolling](./firestore_.firestoresettings.md#firestoresettingsexperimentalforcelongpolling) | boolean | Forces the SDK’s underlying network transport (WebChannel) to use long-polling. Each response from the backend will be closed immediately after the backend sends data (by default responses are kept open in case the backend has more data to send). This avoids incompatibility issues with certain proxies, antivirus software, etc. that incorrectly buffer traffic indefinitely. Use of this option will cause some performance degradation though.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. | | [host](./firestore_.firestoresettings.md#firestoresettingshost) | string | The hostname to connect to. | | [ignoreUndefinedProperties](./firestore_.firestoresettings.md#firestoresettingsignoreundefinedproperties) | boolean | Whether to skip nested properties that are set to undefined during object serialization. If set to true, these properties are skipped and not written to Firestore. If set to false or omitted, the SDK throws an exception when it encounters properties of type undefined. | @@ -48,7 +48,7 @@ cacheSizeBytes?: number; 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. -After having had a default value of `false` since its inception 4 years ago, the default value of this setting was changed in the most recent release to `true`. That is, auto-detection of long polling is now enabled by default. To disable it, set this setting to `false`, and please open a GitHub issue to share your problems with long-polling auto-detection. +After having had a default value of `false` since its inception in 2019, the default value of this setting was changed in mid-2023 to `true`. That is, auto-detection of long polling is now enabled by default. To disable it, set this setting to `false`, and please open a GitHub issue to share the problems that motivated you disabling long-polling auto-detection. Signature: diff --git a/packages/firestore/src/api/settings.ts b/packages/firestore/src/api/settings.ts index 23d08d6dab3..abba7d00da0 100644 --- a/packages/firestore/src/api/settings.ts +++ b/packages/firestore/src/api/settings.ts @@ -88,11 +88,11 @@ export interface FirestoreSettings extends LiteSettings { * detect if long-polling should be used. This is very similar to * `experimentalForceLongPolling`, but only uses long-polling if required. * - * After having had a default value of `false` since its inception 4 years - * ago, the default value of this setting was changed in the most recent - * release to `true`. That is, auto-detection of long polling is now enabled - * by default. To disable it, set this setting to `false`, and please open a - * GitHub issue to share your problems with long-polling auto-detection. + * After having had a default value of `false` since its inception in 2019, + * the default value of this setting was changed in mid-2023 to `true`. That + * is, auto-detection of long polling is now enabled by default. To disable + * it, set this setting to `false`, and please open a GitHub issue to share + * the problems that motivated you disabling long-polling auto-detection. */ experimentalAutoDetectLongPolling?: boolean; }