From 0d7bff1b4c10af9eca70939fccaf9cfa7d663810 Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Thu, 30 Mar 2023 22:17:54 -0400 Subject: [PATCH 01/21] Firestore: add experimentalLongPollingTimeout setting (b/266868871, https://github.com/firebase/firebase-js-sdk/issues/6987) --- common/api-review/firestore.api.md | 1 + packages/firestore/src/api/settings.ts | 19 ++++ packages/firestore/src/core/database_info.ts | 4 + packages/firestore/src/lite-api/components.ts | 1 + packages/firestore/src/lite-api/settings.ts | 44 +++++++++ .../platform/browser/webchannel_connection.ts | 6 ++ .../test/integration/util/internal_helpers.ts | 1 + .../firestore/test/unit/api/database.test.ts | 64 ++++++++++++- .../test/unit/remote/rest_connection.test.ts | 1 + .../test/unit/specs/spec_test_runner.ts | 1 + .../webchannel-wrapper/externs/overrides.js | 3 + packages/webchannel-wrapper/package.json | 4 +- packages/webchannel-wrapper/src/index.d.ts | 1 + yarn.lock | 90 +++++++++---------- 14 files changed, 192 insertions(+), 48 deletions(-) diff --git a/common/api-review/firestore.api.md b/common/api-review/firestore.api.md index 905629f870a..438c0548024 100644 --- a/common/api-review/firestore.api.md +++ b/common/api-review/firestore.api.md @@ -227,6 +227,7 @@ export interface FirestoreSettings { cacheSizeBytes?: number; experimentalAutoDetectLongPolling?: boolean; experimentalForceLongPolling?: boolean; + experimentalLongPollingTimeout?: number; host?: string; ignoreUndefinedProperties?: boolean; localCache?: FirestoreLocalCache; diff --git a/packages/firestore/src/api/settings.ts b/packages/firestore/src/api/settings.ts index 0a9dc9d7ebe..63d05efdf74 100644 --- a/packages/firestore/src/api/settings.ts +++ b/packages/firestore/src/api/settings.ts @@ -92,4 +92,23 @@ export interface FirestoreSettings extends LiteSettings { * cannot be combined with `experimentalForceLongPolling`. */ experimentalAutoDetectLongPolling?: boolean; + + /** + * The desired maximum timeout interval (in milliseconds) to complete a + * long-polling GET response. + * + * By default, when long-polling is used the "hanging GET" request sent by the + * client times out after 30 seconds (30,000 milliseconds). To request a + * different timeout from the server, set this setting with the desired + * timeout. This may be useful, for example, if the buffering proxy that + * necessitated enabling long-polling in the first place has a shorter timeout + * for hanging GET requests, in which case setting the long-polling timeout to + * a shorter value, such as 25 seconds, may fix prematurely-closed hanging GET + * requests. + * + * This setting is only used if `experimentalForceLongPolling` is true or if + * `experimentalAutoDetectLongPolling` is true and the auto-detection + * determined that long-polling was needed. Otherwise, it is ignored. + */ + experimentalLongPollingTimeout?: number; } diff --git a/packages/firestore/src/core/database_info.ts b/packages/firestore/src/core/database_info.ts index 306a9920ea9..2d5da0301de 100644 --- a/packages/firestore/src/core/database_info.ts +++ b/packages/firestore/src/core/database_info.ts @@ -34,6 +34,9 @@ export class DatabaseInfo { * when using WebChannel as the network transport. * @param autoDetectLongPolling - Whether to use the detectBufferingProxy * option when using WebChannel as the network transport. + * @param longPollingTimeout The desired maximum timeout interval (in + * milliseconds) to complete a long-polling GET response; the server _may_ + * ignore this value; may be undefined to use the default (30 seconds). * @param useFetchStreams Whether to use the Fetch API instead of * XMLHTTPRequest */ @@ -45,6 +48,7 @@ export class DatabaseInfo { readonly ssl: boolean, readonly forceLongPolling: boolean, readonly autoDetectLongPolling: boolean, + readonly longPollingTimeout: number | undefined, readonly useFetchStreams: boolean ) {} } diff --git a/packages/firestore/src/lite-api/components.ts b/packages/firestore/src/lite-api/components.ts index 8280d396b0b..43d72154539 100644 --- a/packages/firestore/src/lite-api/components.ts +++ b/packages/firestore/src/lite-api/components.ts @@ -117,6 +117,7 @@ export function makeDatabaseInfo( settings.ssl, settings.experimentalForceLongPolling, settings.experimentalAutoDetectLongPolling, + settings.experimentalLongPollingTimeout, settings.useFetchStreams ); } diff --git a/packages/firestore/src/lite-api/settings.ts b/packages/firestore/src/lite-api/settings.ts index 5f395846720..156557d8e59 100644 --- a/packages/firestore/src/lite-api/settings.ts +++ b/packages/firestore/src/lite-api/settings.ts @@ -29,6 +29,17 @@ import { validateIsNotUsedTogether } from '../util/input_validation'; export const DEFAULT_HOST = 'firestore.googleapis.com'; export const DEFAULT_SSL = true; +// The minimum long-polling timeout is hardcoded on the server. The value here +// should be kept in sync with the value used by the server, as the server will +// silently ignore a value below the minimum and fall back to the default. +// Googlers see http://google3/net/webchannel/internal/webchannel_config.cc;l=118;rcl=510899643 +const MIN_LONG_POLLING_TIMEOUT = 5000; + +// No maximum long-polling timeout is not enforced by the server; however, a +// "reasonable" maximum value is set by the client to provide some guard rails. +// Googlers see b/266868871 for relevant discussion. +const MAX_LONG_POLLING_TIMEOUT = 600000; + /** * Specifies custom configurations for your Cloud Firestore instance. * You must set these before invoking any other methods. @@ -60,6 +71,8 @@ export interface PrivateSettings extends FirestoreSettings { // Used in firestore@exp experimentalAutoDetectLongPolling?: boolean; // Used in firestore@exp + experimentalLongPollingTimeout?: number; + // Used in firestore@exp useFetchStreams?: boolean; localCache?: FirestoreLocalCache; @@ -83,6 +96,8 @@ export class FirestoreSettingsImpl { readonly experimentalAutoDetectLongPolling: boolean; + readonly experimentalLongPollingTimeout: number | undefined; + readonly ignoreUndefinedProperties: boolean; readonly useFetchStreams: boolean; @@ -130,6 +145,7 @@ export class FirestoreSettingsImpl { this.experimentalForceLongPolling = !!settings.experimentalForceLongPolling; this.experimentalAutoDetectLongPolling = !!settings.experimentalAutoDetectLongPolling; + this.experimentalLongPollingTimeout = settings?.experimentalLongPollingTimeout; this.useFetchStreams = !!settings.useFetchStreams; validateIsNotUsedTogether( @@ -138,6 +154,32 @@ export class FirestoreSettingsImpl { 'experimentalAutoDetectLongPolling', settings.experimentalAutoDetectLongPolling ); + + if (typeof this.experimentalLongPollingTimeout === 'number') { + if (! Number.isInteger(this.experimentalLongPollingTimeout)) { + throw new FirestoreError( + Code.INVALID_ARGUMENT, + `invalid long polling timeout: ` + + `${this.experimentalLongPollingTimeout} (must be an integer)` + ); + } + if (this.experimentalLongPollingTimeout < MIN_LONG_POLLING_TIMEOUT) { + throw new FirestoreError( + Code.INVALID_ARGUMENT, + `invalid long polling timeout: ` + + `${this.experimentalLongPollingTimeout} ` + + `(minimum allowed value is ${MIN_LONG_POLLING_TIMEOUT})` + ); + } + if (this.experimentalLongPollingTimeout > MAX_LONG_POLLING_TIMEOUT) { + throw new FirestoreError( + Code.INVALID_ARGUMENT, + `invalid long polling timeout: ` + + `${this.experimentalLongPollingTimeout} ` + + `(maximum allowed value is ${MAX_LONG_POLLING_TIMEOUT})` + ); + } + } } isEqual(other: FirestoreSettingsImpl): boolean { @@ -150,6 +192,8 @@ export class FirestoreSettingsImpl { other.experimentalForceLongPolling && this.experimentalAutoDetectLongPolling === other.experimentalAutoDetectLongPolling && + this.experimentalLongPollingTimeout === + other.experimentalLongPollingTimeout && this.ignoreUndefinedProperties === other.ignoreUndefinedProperties && this.useFetchStreams === other.useFetchStreams ); diff --git a/packages/firestore/src/platform/browser/webchannel_connection.ts b/packages/firestore/src/platform/browser/webchannel_connection.ts index c7eeec0f7f1..977a4e782ab 100644 --- a/packages/firestore/src/platform/browser/webchannel_connection.ts +++ b/packages/firestore/src/platform/browser/webchannel_connection.ts @@ -57,12 +57,14 @@ export class WebChannelConnection extends RestConnection { private readonly forceLongPolling: boolean; private readonly autoDetectLongPolling: boolean; private readonly useFetchStreams: boolean; + private readonly longPollingTimeout: number | undefined; constructor(info: DatabaseInfo) { super(info); this.forceLongPolling = info.forceLongPolling; this.autoDetectLongPolling = info.autoDetectLongPolling; this.useFetchStreams = info.useFetchStreams; + this.longPollingTimeout = info.longPollingTimeout; } protected performRPCRequest( @@ -200,6 +202,10 @@ export class WebChannelConnection extends RestConnection { detectBufferingProxy: this.autoDetectLongPolling }; + if (this.longPollingTimeout) { + request.longPollingTimeout = this.longPollingTimeout; + } + if (this.useFetchStreams) { request.xmlHttpFactory = new FetchXmlHttpFactory({}); } diff --git a/packages/firestore/test/integration/util/internal_helpers.ts b/packages/firestore/test/integration/util/internal_helpers.ts index 58db06b5b85..d2ba19b48e5 100644 --- a/packages/firestore/test/integration/util/internal_helpers.ts +++ b/packages/firestore/test/integration/util/internal_helpers.ts @@ -57,6 +57,7 @@ export function getDefaultDatabaseInfo(): DatabaseInfo { !!DEFAULT_SETTINGS.ssl, !!DEFAULT_SETTINGS.experimentalForceLongPolling, !!DEFAULT_SETTINGS.experimentalAutoDetectLongPolling, + DEFAULT_SETTINGS.experimentalLongPollingTimeout, /*use FetchStreams= */ false ); } diff --git a/packages/firestore/test/unit/api/database.test.ts b/packages/firestore/test/unit/api/database.test.ts index edc1a520f07..8582f0d2bef 100644 --- a/packages/firestore/test/unit/api/database.test.ts +++ b/packages/firestore/test/unit/api/database.test.ts @@ -265,7 +265,7 @@ describe('SnapshotMetadata', () => { }); }); -describe('Settings', () => { +describe.only('Settings', () => { it('can not use mutually exclusive settings together', () => { // Use a new instance of Firestore in order to configure settings. const db = newTestFirestore(); @@ -279,6 +279,68 @@ describe('Settings', () => { ); }); + it('experimentalLongPollingTimeout is undefined by default', () => { + // Use a new instance of Firestore in order to configure settings. + const db = newTestFirestore(); + expect(db._getSettings().experimentalLongPollingTimeout).to.be.undefined; + }); + + it('experimentalLongPollingTimeout minimum value is allowed', () => { + // Use a new instance of Firestore in order to configure settings. + const db = newTestFirestore(); + db._setSettings({experimentalLongPollingTimeout: 5000}); + expect(db._getSettings().experimentalLongPollingTimeout).to.equal(5000); + }); + + it('experimentalLongPollingTimeout maximum value is allowed', () => { + // Use a new instance of Firestore in order to configure settings. + const db = newTestFirestore(); + db._setSettings({experimentalLongPollingTimeout: 600000}); + expect(db._getSettings().experimentalLongPollingTimeout).to.equal(600000); + }); + + it('experimentalLongPollingTimeout typical value is allowed', () => { + // Use a new instance of Firestore in order to configure settings. + const db = newTestFirestore(); + db._setSettings({experimentalLongPollingTimeout: 25000}); + expect(db._getSettings().experimentalLongPollingTimeout).to.equal(25000); + }); + + it('experimentalLongPollingTimeout value of 4999 throws', () => { + // Use a new instance of Firestore in order to configure settings. + const db = newTestFirestore(); + expect(() => db._setSettings({experimentalLongPollingTimeout: 4999})) + .to.throw(/invalid.*timeout.*4999.*\(.*5000.*\)/i); + }); + + it('experimentalLongPollingTimeout value of 0 throws', () => { + // Use a new instance of Firestore in order to configure settings. + const db = newTestFirestore(); + expect(() => db._setSettings({experimentalLongPollingTimeout: 0})) + .to.throw(/invalid.*timeout.*0.*\(.*5000.*\)/i); + }); + + it('experimentalLongPollingTimeout value of -1 throws', () => { + // Use a new instance of Firestore in order to configure settings. + const db = newTestFirestore(); + expect(() => db._setSettings({experimentalLongPollingTimeout: -1})) + .to.throw(/invalid.*timeout.*-1.*\(.*5000.*\)/i); + }); + + it('experimentalLongPollingTimeout value of 600001 throws', () => { + // Use a new instance of Firestore in order to configure settings. + const db = newTestFirestore(); + expect(() => db._setSettings({experimentalLongPollingTimeout: 600001})) + .to.throw(/invalid.*timeout.*600001.*\(.*600000.*\)/i); + }); + + it('experimentalLongPollingTimeout non-integral value throws', () => { + // Use a new instance of Firestore in order to configure settings. + const db = newTestFirestore(); + expect(() => db._setSettings({experimentalLongPollingTimeout: 123.456})) + .to.throw(/invalid.*timeout.*123.456.*\(.*integer.*\)/i); + }); + it('gets settings from useEmulator', () => { // Use a new instance of Firestore in order to configure settings. const db = newTestFirestore(); diff --git a/packages/firestore/test/unit/remote/rest_connection.test.ts b/packages/firestore/test/unit/remote/rest_connection.test.ts index 8266ce74cf2..0f89d23928f 100644 --- a/packages/firestore/test/unit/remote/rest_connection.test.ts +++ b/packages/firestore/test/unit/remote/rest_connection.test.ts @@ -65,6 +65,7 @@ describe('RestConnection', () => { /*ssl=*/ false, /*forceLongPolling=*/ false, /*autoDetectLongPolling=*/ false, + /*longPollingTimeout=*/ undefined, /*useFetchStreams=*/ false ); const connection = new TestRestConnection(testDatabaseInfo); diff --git a/packages/firestore/test/unit/specs/spec_test_runner.ts b/packages/firestore/test/unit/specs/spec_test_runner.ts index f07a0c4ceb5..072ca41e04e 100644 --- a/packages/firestore/test/unit/specs/spec_test_runner.ts +++ b/packages/firestore/test/unit/specs/spec_test_runner.ts @@ -273,6 +273,7 @@ abstract class TestRunner { /*ssl=*/ false, /*forceLongPolling=*/ false, /*autoDetectLongPolling=*/ false, + /*longPollingTimeout=*/ undefined, /*useFetchStreams=*/ false ); diff --git a/packages/webchannel-wrapper/externs/overrides.js b/packages/webchannel-wrapper/externs/overrides.js index f40853ec6a1..bccac74db64 100644 --- a/packages/webchannel-wrapper/externs/overrides.js +++ b/packages/webchannel-wrapper/externs/overrides.js @@ -69,6 +69,9 @@ goog.net.WebChannel.Options.detectBufferingProxy; /** @type {unknown} */ goog.net.WebChannel.Options.xmlHttpFactory; +/** @type {number|undefined} */ +goog.net.WebChannel.Options.longPollingTimeout; + goog.labs.net.webChannel.requestStats.Event = {}; goog.labs.net.webChannel.requestStats.Event.STAT_EVENT; diff --git a/packages/webchannel-wrapper/package.json b/packages/webchannel-wrapper/package.json index 35217bc539f..4e63cb3ab15 100644 --- a/packages/webchannel-wrapper/package.json +++ b/packages/webchannel-wrapper/package.json @@ -27,8 +27,8 @@ "license": "Apache-2.0", "devDependencies": { "@rollup/plugin-commonjs": "21.1.0", - "google-closure-compiler": "20220301.0.0", - "google-closure-library": "20220301.0.0", + "google-closure-compiler": "20230228.0.0", + "google-closure-library": "20230228.0.0", "gulp": "4.0.2", "gulp-sourcemaps": "3.0.0", "rollup": "2.79.1", diff --git a/packages/webchannel-wrapper/src/index.d.ts b/packages/webchannel-wrapper/src/index.d.ts index 4a1f45f1d70..e275771c5a0 100644 --- a/packages/webchannel-wrapper/src/index.d.ts +++ b/packages/webchannel-wrapper/src/index.d.ts @@ -101,6 +101,7 @@ export interface WebChannelOptions { encodeInitMessageHeaders?: boolean; forceLongPolling?: boolean; detectBufferingProxy?: boolean; + longPollingTimeout?: number; fastHandshake?: boolean; disableRedac?: boolean; clientProfile?: string; diff --git a/yarn.lock b/yarn.lock index 2864857a0eb..51f2b83dd3d 100644 --- a/yarn.lock +++ b/yarn.lock @@ -5697,16 +5697,7 @@ chainsaw@~0.1.0: dependencies: traverse ">=0.3.0 <0.4" -chalk@2.x, chalk@^2.0.0, chalk@^2.0.1, chalk@^2.1.0, chalk@^2.3.0, chalk@^2.4.1, chalk@^2.4.2: - version "2.4.2" - resolved "https://registry.npmjs.org/chalk/-/chalk-2.4.2.tgz#cd42541677a54333cf541a49108c1432b44c9424" - integrity sha512-Mti+f9lpJNcwF4tWV8/OrTTtF1gZi+f8FqlyAdouralcFWFQWF2+NgCHShjkCb+IFBLq9buZwE1xckQU4peSuQ== - dependencies: - ansi-styles "^3.2.1" - escape-string-regexp "^1.0.5" - supports-color "^5.3.0" - -chalk@4.1.2, chalk@^4.0.0, chalk@^4.1.0, chalk@^4.1.1, chalk@^4.1.2: +chalk@4.1.2, chalk@4.x, chalk@^4.0.0, chalk@^4.1.0, chalk@^4.1.1, chalk@^4.1.2: version "4.1.2" resolved "https://registry.npmjs.org/chalk/-/chalk-4.1.2.tgz#aac4e2b7734a740867aeb16bf02aad556a1e7a01" integrity sha512-oKnbhFyRIXpUuez8iBMmyEa4nbj4IOQyuhc/wy9kY7/WVPcwIO9VA668Pu8RkO7+0G76SLROeyw9CpQ061i4mA== @@ -5725,6 +5716,15 @@ chalk@^1.0.0, chalk@^1.1.1, chalk@^1.1.3: strip-ansi "^3.0.0" supports-color "^2.0.0" +chalk@^2.0.0, chalk@^2.0.1, chalk@^2.1.0, chalk@^2.3.0, chalk@^2.4.1, chalk@^2.4.2: + version "2.4.2" + resolved "https://registry.npmjs.org/chalk/-/chalk-2.4.2.tgz#cd42541677a54333cf541a49108c1432b44c9424" + integrity sha512-Mti+f9lpJNcwF4tWV8/OrTTtF1gZi+f8FqlyAdouralcFWFQWF2+NgCHShjkCb+IFBLq9buZwE1xckQU4peSuQ== + dependencies: + ansi-styles "^3.2.1" + escape-string-regexp "^1.0.5" + supports-color "^5.3.0" + chalk@^3.0.0: version "3.0.0" resolved "https://registry.npmjs.org/chalk/-/chalk-3.0.0.tgz#3f73c2bf526591f574cc492c51e2456349f844e4" @@ -9149,45 +9149,45 @@ google-auth-library@^8.0.2: jws "^4.0.0" lru-cache "^6.0.0" -google-closure-compiler-java@^20220301.0.0: - version "20220301.0.0" - resolved "https://registry.npmjs.org/google-closure-compiler-java/-/google-closure-compiler-java-20220301.0.0.tgz#6283bad6991ae9cfb3a9fdf72bbd7bf0c8f21fb6" - integrity sha512-kv5oaUI4xn3qWYWtRHRqbm314kesfeFlCxiFRcvBIx13mKfR0qvbOkgajLpSM6nb3voNM/E9MB9mfvHJ9XIXSg== - -google-closure-compiler-linux@^20220301.0.0: - version "20220301.0.0" - resolved "https://registry.npmjs.org/google-closure-compiler-linux/-/google-closure-compiler-linux-20220301.0.0.tgz#3ac8cd1cb51d703a89bc49c239df4c10b57f37bb" - integrity sha512-N2D0SRnxZ7kqdoZ2WsmLIjmizR4Xr0HaUYDK2RCOtsV21RYV8OR2u0ATp7aXhYy8WfxvYH478Ehvmc9Uzy986A== - -google-closure-compiler-osx@^20220301.0.0: - version "20220301.0.0" - resolved "https://registry.npmjs.org/google-closure-compiler-osx/-/google-closure-compiler-osx-20220301.0.0.tgz#1a49eb1d78b6bfb90ebe51c24a7151cee4f319a3" - integrity sha512-Xqf0m5takwfv43ML4aODJxmAsAZQMTMo683gyRs0APAecncs+YKxaDPMH+pQAdI3HPY2QsvkarlunAp0HSwU5A== - -google-closure-compiler-windows@^20220301.0.0: - version "20220301.0.0" - resolved "https://registry.npmjs.org/google-closure-compiler-windows/-/google-closure-compiler-windows-20220301.0.0.tgz#b09df91a789e458eb9ebf054a9bb2d2b29622b6f" - integrity sha512-s+FU/vcpLTEgx8MCMgj0STCYkVk7syzF9KqiYPOTtbTD9ra99HPe/CEuQG7iJ3Fty9dhm9zEaetv4Dp4Wr6x+Q== - -google-closure-compiler@20220301.0.0: - version "20220301.0.0" - resolved "https://registry.npmjs.org/google-closure-compiler/-/google-closure-compiler-20220301.0.0.tgz#1c4f56076ae5b2c900a91d0a72515f7ee7f5d3cd" - integrity sha512-+yAqhufKIWddg587tnvRll92eLJQIlzINmgr1h5gLXZVioY3svrSYKH4TZiUuNj0UnVFoK0o1YuW122x+iFl2g== - dependencies: - chalk "2.x" - google-closure-compiler-java "^20220301.0.0" +google-closure-compiler-java@^20230228.0.0: + version "20230228.0.0" + resolved "https://registry.npmjs.org/google-closure-compiler-java/-/google-closure-compiler-java-20230228.0.0.tgz#d74036a40917166e7b4fb6cf5e8878eada2ab93f" + integrity sha512-t0sXYJbhfkuNTF6zniwrTv4gLap620D32v6GwBJQzlYUg0lb7yQHN9KswwqBsuuO917cPNwW4okI0O40G7GrMQ== + +google-closure-compiler-linux@^20230228.0.0: + version "20230228.0.0" + resolved "https://registry.npmjs.org/google-closure-compiler-linux/-/google-closure-compiler-linux-20230228.0.0.tgz#17cb6187015e0e2ae8c2dd5b560228fdf5625818" + integrity sha512-5YLxfWS8lvHkD/a0+pitTuDV1X9QPBToGQ5mnLFg7HcbBR1w6I5ZKHjl7FAsAOHEXYwIrStwwaLzrNzbolrZLg== + +google-closure-compiler-osx@^20230228.0.0: + version "20230228.0.0" + resolved "https://registry.npmjs.org/google-closure-compiler-osx/-/google-closure-compiler-osx-20230228.0.0.tgz#245caa71b2eff3c5f4a4ec2046b2dd766c5fbe2f" + integrity sha512-ORveHpHuNhJEJIGir35+xP4UuBOldSO8XeOwJV5yunUhZAPzR4aixdTdtm6i0GsqW4/Eu2cjcHrkIR3eFCcwSg== + +google-closure-compiler-windows@^20230228.0.0: + version "20230228.0.0" + resolved "https://registry.npmjs.org/google-closure-compiler-windows/-/google-closure-compiler-windows-20230228.0.0.tgz#dff4afcd6d21a831b38a9bdb873eed0daca79807" + integrity sha512-xKMjUq6JwEOFqS97S86TWkn+BMiDHjP85mMgAmR8vRmKxgfHIyxMcr+RlMz0msgY9jedgj119KXyOe32lIQTjA== + +google-closure-compiler@20230228.0.0: + version "20230228.0.0" + resolved "https://registry.npmjs.org/google-closure-compiler/-/google-closure-compiler-20230228.0.0.tgz#a891408dca350bb7fe56ee50e2b4ee97f3a740d6" + integrity sha512-jFI4QNZgM4WhNIoaRNwA5kHq6n6NKSWZj3N9HgRsJE9bN4LUrkIURI+svChbEp/WmGh3Bt3o3/5kUlOOWyCo3Q== + dependencies: + chalk "4.x" + google-closure-compiler-java "^20230228.0.0" minimist "1.x" vinyl "2.x" vinyl-sourcemaps-apply "^0.2.0" optionalDependencies: - google-closure-compiler-linux "^20220301.0.0" - google-closure-compiler-osx "^20220301.0.0" - google-closure-compiler-windows "^20220301.0.0" - -google-closure-library@20220301.0.0: - version "20220301.0.0" - resolved "https://registry.npmjs.org/google-closure-library/-/google-closure-library-20220301.0.0.tgz#c9aaa99218f949b1f914a86f2a4529dea20e2e47" - integrity sha512-GRRBfG80JPqkKkTxiRoVr/x4UmnPW2aeA72NH0zapPtrvSkAOCzfJFrdudLrAJJtXPdSE65+CkYrpZX8tP0mCQ== + google-closure-compiler-linux "^20230228.0.0" + google-closure-compiler-osx "^20230228.0.0" + google-closure-compiler-windows "^20230228.0.0" + +google-closure-library@20230228.0.0: + version "20230228.0.0" + resolved "https://registry.npmjs.org/google-closure-library/-/google-closure-library-20230228.0.0.tgz#8ef2f9de391c69e8b8e26e7c0bcd413b0b6bb605" + integrity sha512-yIe7gpacdmfM8n6Cvswajw7MgiWYwr46/1HHVCYKthyrfEBabDa1zCH6BPJJi07cnN7ImCyKtOGZXz6EAwtXBg== google-gax@^3.0.1: version "3.1.3" From bf5d4097d0838a708c16dd0f4908acf5193f85b5 Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Thu, 13 Apr 2023 15:13:19 -0400 Subject: [PATCH 02/21] experimentalLongPollingTimeout -> experimentalLongPollingOptions.idleHttpRequestTimeoutSeconds WIP --- common/api-review/firestore.api.md | 4 +- packages/firestore/src/api/settings.ts | 36 +++++++------ packages/firestore/src/core/database_info.ts | 2 +- packages/firestore/src/lite-api/settings.ts | 40 ++++++++------- .../test/integration/util/internal_helpers.ts | 2 +- .../firestore/test/unit/api/database.test.ts | 50 +++++++++---------- 6 files changed, 74 insertions(+), 60 deletions(-) diff --git a/common/api-review/firestore.api.md b/common/api-review/firestore.api.md index 438c0548024..97dcc46ec87 100644 --- a/common/api-review/firestore.api.md +++ b/common/api-review/firestore.api.md @@ -227,7 +227,9 @@ export interface FirestoreSettings { cacheSizeBytes?: number; experimentalAutoDetectLongPolling?: boolean; experimentalForceLongPolling?: boolean; - experimentalLongPollingTimeout?: number; + experimentalLongPollingOptions?: { + idleHttpRequestTimeoutSeconds?: number; + }; host?: string; ignoreUndefinedProperties?: boolean; localCache?: FirestoreLocalCache; diff --git a/packages/firestore/src/api/settings.ts b/packages/firestore/src/api/settings.ts index 80e39b93f7f..dcef8fadf84 100644 --- a/packages/firestore/src/api/settings.ts +++ b/packages/firestore/src/api/settings.ts @@ -94,21 +94,29 @@ export interface FirestoreSettings extends LiteSettings { experimentalAutoDetectLongPolling?: boolean; /** - * The desired maximum timeout interval (in milliseconds) to complete a - * long-polling GET response. + * Options for long polling when it is used. * - * By default, when long-polling is used the "hanging GET" request sent by the - * client times out after 30 seconds (30,000 milliseconds). To request a - * different timeout from the server, set this setting with the desired - * timeout. This may be useful, for example, if the buffering proxy that - * necessitated enabling long-polling in the first place has a shorter timeout - * for hanging GET requests, in which case setting the long-polling timeout to - * a shorter value, such as 25 seconds, may fix prematurely-closed hanging GET - * requests. - * - * This setting is only used if `experimentalForceLongPolling` is true or if + * These options are only used if `experimentalForceLongPolling` is true or if * `experimentalAutoDetectLongPolling` is true and the auto-detection - * determined that long-polling was needed. Otherwise, it is ignored. + * determined that long-polling was needed. Otherwise, they have no effect. */ - experimentalLongPollingTimeout?: number; + experimentalLongPollingOptions?: { + + /** + * The desired maximum timeout interval (in seconds) to complete a long + * polling GET response. Valid values are integers between 5 and 30, + * inclusive. + * + * By default, when long polling is used the "hanging GET" request sent by + * the client times out after 30 seconds. To request a different timeout + * from the server, set this setting with the desired timeout. This may be + * useful, for example, if the buffering proxy that necessitated enabling + * long polling in the first place has a shorter timeout for hanging GET + * requests, in which case setting the long-polling timeout to a shorter + * value, such as 25 seconds, may fix prematurely-closed hanging GET + * requests. + */ + idleHttpRequestTimeoutSeconds?: number; + + } } diff --git a/packages/firestore/src/core/database_info.ts b/packages/firestore/src/core/database_info.ts index 2d5da0301de..d2966392285 100644 --- a/packages/firestore/src/core/database_info.ts +++ b/packages/firestore/src/core/database_info.ts @@ -48,7 +48,7 @@ export class DatabaseInfo { readonly ssl: boolean, readonly forceLongPolling: boolean, readonly autoDetectLongPolling: boolean, - readonly longPollingTimeout: number | undefined, + readonly longPollingOptions: { idleHttpRequestTimeoutSeconds?: number } | undefined, readonly useFetchStreams: boolean ) {} } diff --git a/packages/firestore/src/lite-api/settings.ts b/packages/firestore/src/lite-api/settings.ts index 156557d8e59..53612795d43 100644 --- a/packages/firestore/src/lite-api/settings.ts +++ b/packages/firestore/src/lite-api/settings.ts @@ -33,12 +33,12 @@ export const DEFAULT_SSL = true; // should be kept in sync with the value used by the server, as the server will // silently ignore a value below the minimum and fall back to the default. // Googlers see http://google3/net/webchannel/internal/webchannel_config.cc;l=118;rcl=510899643 -const MIN_LONG_POLLING_TIMEOUT = 5000; +const MIN_LONG_POLLING_TIMEOUT_SECONDS = 5; -// No maximum long-polling timeout is not enforced by the server; however, a -// "reasonable" maximum value is set by the client to provide some guard rails. +// No maximum long-polling timeout is configured in the server, and defaults to +// 30 seconds, which is what Watch appears to use. // Googlers see b/266868871 for relevant discussion. -const MAX_LONG_POLLING_TIMEOUT = 600000; +const MAX_LONG_POLLING_TIMEOUT_SECONDS = 30; /** * Specifies custom configurations for your Cloud Firestore instance. @@ -71,7 +71,9 @@ export interface PrivateSettings extends FirestoreSettings { // Used in firestore@exp experimentalAutoDetectLongPolling?: boolean; // Used in firestore@exp - experimentalLongPollingTimeout?: number; + experimentalLongPollingOptions?: { + idleHttpRequestTimeoutSeconds?: number + }; // Used in firestore@exp useFetchStreams?: boolean; @@ -96,7 +98,9 @@ export class FirestoreSettingsImpl { readonly experimentalAutoDetectLongPolling: boolean; - readonly experimentalLongPollingTimeout: number | undefined; + readonly experimentalLongPollingOptions?: { + idleHttpRequestTimeoutSeconds?: number + }; readonly ignoreUndefinedProperties: boolean; @@ -145,7 +149,7 @@ export class FirestoreSettingsImpl { this.experimentalForceLongPolling = !!settings.experimentalForceLongPolling; this.experimentalAutoDetectLongPolling = !!settings.experimentalAutoDetectLongPolling; - this.experimentalLongPollingTimeout = settings?.experimentalLongPollingTimeout; + this.experimentalLongPollingOptions = settings?.experimentalLongPollingOptions; this.useFetchStreams = !!settings.useFetchStreams; validateIsNotUsedTogether( @@ -155,28 +159,29 @@ export class FirestoreSettingsImpl { settings.experimentalAutoDetectLongPolling ); - if (typeof this.experimentalLongPollingTimeout === 'number') { - if (! Number.isInteger(this.experimentalLongPollingTimeout)) { + const experimentalLongPollingTimeout = this.experimentalLongPollingOptions?.idleHttpRequestTimeoutSeconds; + if (experimentalLongPollingTimeout !== undefined) { + if (! Number.isInteger(experimentalLongPollingTimeout)) { throw new FirestoreError( Code.INVALID_ARGUMENT, `invalid long polling timeout: ` + - `${this.experimentalLongPollingTimeout} (must be an integer)` + `${experimentalLongPollingTimeout} (must be an integer)` ); } - if (this.experimentalLongPollingTimeout < MIN_LONG_POLLING_TIMEOUT) { + if (experimentalLongPollingTimeout < MIN_LONG_POLLING_TIMEOUT_SECONDS) { throw new FirestoreError( Code.INVALID_ARGUMENT, `invalid long polling timeout: ` + - `${this.experimentalLongPollingTimeout} ` + - `(minimum allowed value is ${MIN_LONG_POLLING_TIMEOUT})` + `${experimentalLongPollingTimeout} ` + + `(minimum allowed value is ${MIN_LONG_POLLING_TIMEOUT_SECONDS})` ); } - if (this.experimentalLongPollingTimeout > MAX_LONG_POLLING_TIMEOUT) { + if (experimentalLongPollingTimeout > MAX_LONG_POLLING_TIMEOUT_SECONDS) { throw new FirestoreError( Code.INVALID_ARGUMENT, `invalid long polling timeout: ` + - `${this.experimentalLongPollingTimeout} ` + - `(maximum allowed value is ${MAX_LONG_POLLING_TIMEOUT})` + `${experimentalLongPollingTimeout} ` + + `(maximum allowed value is ${MAX_LONG_POLLING_TIMEOUT_SECONDS})` ); } } @@ -192,8 +197,7 @@ export class FirestoreSettingsImpl { other.experimentalForceLongPolling && this.experimentalAutoDetectLongPolling === other.experimentalAutoDetectLongPolling && - this.experimentalLongPollingTimeout === - other.experimentalLongPollingTimeout && + (this.experimentalLongPollingOptions?.idleHttpRequestTimeoutSeconds === other.experimentalLongPollingOptions?.idleHttpRequestTimeoutSeconds) && this.ignoreUndefinedProperties === other.ignoreUndefinedProperties && this.useFetchStreams === other.useFetchStreams ); diff --git a/packages/firestore/test/integration/util/internal_helpers.ts b/packages/firestore/test/integration/util/internal_helpers.ts index d2ba19b48e5..dfcaedc47d6 100644 --- a/packages/firestore/test/integration/util/internal_helpers.ts +++ b/packages/firestore/test/integration/util/internal_helpers.ts @@ -57,7 +57,7 @@ export function getDefaultDatabaseInfo(): DatabaseInfo { !!DEFAULT_SETTINGS.ssl, !!DEFAULT_SETTINGS.experimentalForceLongPolling, !!DEFAULT_SETTINGS.experimentalAutoDetectLongPolling, - DEFAULT_SETTINGS.experimentalLongPollingTimeout, + DEFAULT_SETTINGS.experimentalLongPollingOptions, /*use FetchStreams= */ false ); } diff --git a/packages/firestore/test/unit/api/database.test.ts b/packages/firestore/test/unit/api/database.test.ts index 8582f0d2bef..76930fcda84 100644 --- a/packages/firestore/test/unit/api/database.test.ts +++ b/packages/firestore/test/unit/api/database.test.ts @@ -279,65 +279,65 @@ describe.only('Settings', () => { ); }); - it('experimentalLongPollingTimeout is undefined by default', () => { + it('idleHttpRequestTimeoutSeconds is undefined by default', () => { // Use a new instance of Firestore in order to configure settings. const db = newTestFirestore(); - expect(db._getSettings().experimentalLongPollingTimeout).to.be.undefined; + expect(db._getSettings().experimentalLongPollingOptions?.idleHttpRequestTimeoutSeconds).to.be.undefined; }); - it('experimentalLongPollingTimeout minimum value is allowed', () => { + it('idleHttpRequestTimeoutSeconds minimum value is allowed', () => { // Use a new instance of Firestore in order to configure settings. const db = newTestFirestore(); - db._setSettings({experimentalLongPollingTimeout: 5000}); - expect(db._getSettings().experimentalLongPollingTimeout).to.equal(5000); + db._setSettings({experimentalLongPollingOptions: { idleHttpRequestTimeoutSeconds: 5 }}); + expect(db._getSettings().experimentalLongPollingOptions?.idleHttpRequestTimeoutSeconds).to.equal(5); }); - it('experimentalLongPollingTimeout maximum value is allowed', () => { + it('idleHttpRequestTimeoutSeconds maximum value is allowed', () => { // Use a new instance of Firestore in order to configure settings. const db = newTestFirestore(); - db._setSettings({experimentalLongPollingTimeout: 600000}); - expect(db._getSettings().experimentalLongPollingTimeout).to.equal(600000); + db._setSettings({experimentalLongPollingOptions: { idleHttpRequestTimeoutSeconds: 30 }}); + expect(db._getSettings().experimentalLongPollingOptions?.idleHttpRequestTimeoutSeconds).to.equal(30); }); - it('experimentalLongPollingTimeout typical value is allowed', () => { + it('idleHttpRequestTimeoutSeconds typical value is allowed', () => { // Use a new instance of Firestore in order to configure settings. const db = newTestFirestore(); - db._setSettings({experimentalLongPollingTimeout: 25000}); - expect(db._getSettings().experimentalLongPollingTimeout).to.equal(25000); + db._setSettings({experimentalLongPollingOptions: { idleHttpRequestTimeoutSeconds: 25 }}); + expect(db._getSettings().experimentalLongPollingOptions?.idleHttpRequestTimeoutSeconds).to.equal(25); }); - it('experimentalLongPollingTimeout value of 4999 throws', () => { + it('idleHttpRequestTimeoutSeconds value one less than minimum throws', () => { // Use a new instance of Firestore in order to configure settings. const db = newTestFirestore(); - expect(() => db._setSettings({experimentalLongPollingTimeout: 4999})) - .to.throw(/invalid.*timeout.*4999.*\(.*5000.*\)/i); + expect(() => db._setSettings({experimentalLongPollingOptions: { idleHttpRequestTimeoutSeconds: 4 }})) + .to.throw(/invalid.*timeout.*4.*\(.*5.*\)/i); }); - it('experimentalLongPollingTimeout value of 0 throws', () => { + it('idleHttpRequestTimeoutSeconds value one more than maximum throws', () => { // Use a new instance of Firestore in order to configure settings. const db = newTestFirestore(); - expect(() => db._setSettings({experimentalLongPollingTimeout: 0})) - .to.throw(/invalid.*timeout.*0.*\(.*5000.*\)/i); + expect(() => db._setSettings({experimentalLongPollingOptions: { idleHttpRequestTimeoutSeconds: 31 }})) + .to.throw(/invalid.*timeout.*31.*\(.*30.*\)/i); }); - it('experimentalLongPollingTimeout value of -1 throws', () => { + it('idleHttpRequestTimeoutSeconds value of 0 throws', () => { // Use a new instance of Firestore in order to configure settings. const db = newTestFirestore(); - expect(() => db._setSettings({experimentalLongPollingTimeout: -1})) - .to.throw(/invalid.*timeout.*-1.*\(.*5000.*\)/i); + expect(() => db._setSettings({experimentalLongPollingOptions: { idleHttpRequestTimeoutSeconds: 0 }})) + .to.throw(/invalid.*timeout.*0.*\(.*5.*\)/i); }); - it('experimentalLongPollingTimeout value of 600001 throws', () => { + it('idleHttpRequestTimeoutSeconds value of -1 throws', () => { // Use a new instance of Firestore in order to configure settings. const db = newTestFirestore(); - expect(() => db._setSettings({experimentalLongPollingTimeout: 600001})) - .to.throw(/invalid.*timeout.*600001.*\(.*600000.*\)/i); + expect(() => db._setSettings({experimentalLongPollingOptions: { idleHttpRequestTimeoutSeconds: -1 }})) + .to.throw(/invalid.*timeout.*-1.*\(.*5.*\)/i); }); - it('experimentalLongPollingTimeout non-integral value throws', () => { + it('idleHttpRequestTimeoutSeconds non-integral value throws', () => { // Use a new instance of Firestore in order to configure settings. const db = newTestFirestore(); - expect(() => db._setSettings({experimentalLongPollingTimeout: 123.456})) + expect(() => db._setSettings({experimentalLongPollingOptions: { idleHttpRequestTimeoutSeconds: 123.456 }})) .to.throw(/invalid.*timeout.*123.456.*\(.*integer.*\)/i); }); From a7574d41a9fc2d07f710a70b3343a2a7bdf39336 Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Fri, 14 Apr 2023 00:42:57 -0400 Subject: [PATCH 03/21] ExperimentalLongPollingOptions interface, part 1 [no ci] --- packages/firestore/src/api.ts | 2 +- packages/firestore/src/api/settings.ts | 43 ++++++++++++++++---------- 2 files changed, 27 insertions(+), 18 deletions(-) diff --git a/packages/firestore/src/api.ts b/packages/firestore/src/api.ts index f06bf1bc568..0ee3f3a01da 100644 --- a/packages/firestore/src/api.ts +++ b/packages/firestore/src/api.ts @@ -75,7 +75,7 @@ export { TaskState } from './api/bundle'; -export { FirestoreSettings, PersistenceSettings } from './api/settings'; +export { ExperimentalLongPollingOptions, FirestoreSettings, PersistenceSettings } from './api/settings'; export { DocumentChange, diff --git a/packages/firestore/src/api/settings.ts b/packages/firestore/src/api/settings.ts index dcef8fadf84..7c11632107a 100644 --- a/packages/firestore/src/api/settings.ts +++ b/packages/firestore/src/api/settings.ts @@ -100,23 +100,32 @@ export interface FirestoreSettings extends LiteSettings { * `experimentalAutoDetectLongPolling` is true and the auto-detection * determined that long-polling was needed. Otherwise, they have no effect. */ - experimentalLongPollingOptions?: { + experimentalLongPollingOptions?: ExperimentalLongPollingOptions; +} + +/** + * Options for long polling when it is used. + * + * See `FirestoreSettings.experimentalAutoDetectLongPolling`, + * `FirestoreSettings.experimentalForceLongPolling`, and + * `FirestoreSettings.experimentalLongPollingOptions`. + */ +export interface ExperimentalLongPollingOptions { - /** - * The desired maximum timeout interval (in seconds) to complete a long - * polling GET response. Valid values are integers between 5 and 30, - * inclusive. - * - * By default, when long polling is used the "hanging GET" request sent by - * the client times out after 30 seconds. To request a different timeout - * from the server, set this setting with the desired timeout. This may be - * useful, for example, if the buffering proxy that necessitated enabling - * long polling in the first place has a shorter timeout for hanging GET - * requests, in which case setting the long-polling timeout to a shorter - * value, such as 25 seconds, may fix prematurely-closed hanging GET - * requests. - */ - idleHttpRequestTimeoutSeconds?: number; + /** + * The desired maximum timeout interval (in seconds) to complete a long + * polling GET response. Valid values are integers between 5 and 30, + * inclusive. + * + * By default, when long polling is used the "hanging GET" request sent by + * the client times out after 30 seconds. To request a different timeout + * from the server, set this setting with the desired timeout. This may be + * useful, for example, if the buffering proxy that necessitated enabling + * long polling in the first place has a shorter timeout for hanging GET + * requests, in which case setting the long-polling timeout to a shorter + * value, such as 25 seconds, may fix prematurely-closed hanging GET + * requests. + */ + idleHttpRequestTimeoutSeconds?: number; - } } From 6ce1d0145c8ad7f20b104e8277c202ddf5ffd47b Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Fri, 14 Apr 2023 00:57:23 -0400 Subject: [PATCH 04/21] ExperimentalLongPollingOptions interface, part 2 [no ci] --- packages/firestore/src/api.ts | 6 +- packages/firestore/src/api/settings.ts | 2 - packages/firestore/src/core/database_info.ts | 4 +- packages/firestore/src/lite-api/components.ts | 8 ++- packages/firestore/src/lite-api/settings.ts | 25 ++++--- .../platform/browser/webchannel_connection.ts | 2 +- .../firestore/test/unit/api/database.test.ts | 71 ++++++++++++++----- 7 files changed, 82 insertions(+), 36 deletions(-) diff --git a/packages/firestore/src/api.ts b/packages/firestore/src/api.ts index 0ee3f3a01da..082fe37db0b 100644 --- a/packages/firestore/src/api.ts +++ b/packages/firestore/src/api.ts @@ -75,7 +75,11 @@ export { TaskState } from './api/bundle'; -export { ExperimentalLongPollingOptions, FirestoreSettings, PersistenceSettings } from './api/settings'; +export { + ExperimentalLongPollingOptions, + FirestoreSettings, + PersistenceSettings +} from './api/settings'; export { DocumentChange, diff --git a/packages/firestore/src/api/settings.ts b/packages/firestore/src/api/settings.ts index 7c11632107a..9dc9572f85a 100644 --- a/packages/firestore/src/api/settings.ts +++ b/packages/firestore/src/api/settings.ts @@ -111,7 +111,6 @@ export interface FirestoreSettings extends LiteSettings { * `FirestoreSettings.experimentalLongPollingOptions`. */ export interface ExperimentalLongPollingOptions { - /** * The desired maximum timeout interval (in seconds) to complete a long * polling GET response. Valid values are integers between 5 and 30, @@ -127,5 +126,4 @@ export interface ExperimentalLongPollingOptions { * requests. */ idleHttpRequestTimeoutSeconds?: number; - } diff --git a/packages/firestore/src/core/database_info.ts b/packages/firestore/src/core/database_info.ts index d2966392285..766225f6a59 100644 --- a/packages/firestore/src/core/database_info.ts +++ b/packages/firestore/src/core/database_info.ts @@ -36,7 +36,7 @@ export class DatabaseInfo { * option when using WebChannel as the network transport. * @param longPollingTimeout The desired maximum timeout interval (in * milliseconds) to complete a long-polling GET response; the server _may_ - * ignore this value; may be undefined to use the default (30 seconds). + * ignore this value; may be null to use the default (30 seconds). * @param useFetchStreams Whether to use the Fetch API instead of * XMLHTTPRequest */ @@ -48,7 +48,7 @@ export class DatabaseInfo { readonly ssl: boolean, readonly forceLongPolling: boolean, readonly autoDetectLongPolling: boolean, - readonly longPollingOptions: { idleHttpRequestTimeoutSeconds?: number } | undefined, + readonly longPollingTimeout: number | null, readonly useFetchStreams: boolean ) {} } diff --git a/packages/firestore/src/lite-api/components.ts b/packages/firestore/src/lite-api/components.ts index 43d72154539..e11caa63394 100644 --- a/packages/firestore/src/lite-api/components.ts +++ b/packages/firestore/src/lite-api/components.ts @@ -117,7 +117,13 @@ export function makeDatabaseInfo( settings.ssl, settings.experimentalForceLongPolling, settings.experimentalAutoDetectLongPolling, - settings.experimentalLongPollingTimeout, + getLongPollingTimeout(settings), settings.useFetchStreams ); } + +function getLongPollingTimeout(settings: FirestoreSettingsImpl): number | null { + const value = + settings.experimentalLongPollingOptions?.idleHttpRequestTimeoutSeconds; + return value === undefined ? null : value * 1000; +} diff --git a/packages/firestore/src/lite-api/settings.ts b/packages/firestore/src/lite-api/settings.ts index 53612795d43..9098c6e22ea 100644 --- a/packages/firestore/src/lite-api/settings.ts +++ b/packages/firestore/src/lite-api/settings.ts @@ -72,7 +72,7 @@ export interface PrivateSettings extends FirestoreSettings { experimentalAutoDetectLongPolling?: boolean; // Used in firestore@exp experimentalLongPollingOptions?: { - idleHttpRequestTimeoutSeconds?: number + idleHttpRequestTimeoutSeconds?: number; }; // Used in firestore@exp useFetchStreams?: boolean; @@ -99,7 +99,7 @@ export class FirestoreSettingsImpl { readonly experimentalAutoDetectLongPolling: boolean; readonly experimentalLongPollingOptions?: { - idleHttpRequestTimeoutSeconds?: number + idleHttpRequestTimeoutSeconds?: number; }; readonly ignoreUndefinedProperties: boolean; @@ -149,7 +149,8 @@ export class FirestoreSettingsImpl { this.experimentalForceLongPolling = !!settings.experimentalForceLongPolling; this.experimentalAutoDetectLongPolling = !!settings.experimentalAutoDetectLongPolling; - this.experimentalLongPollingOptions = settings?.experimentalLongPollingOptions; + this.experimentalLongPollingOptions = + settings.experimentalLongPollingOptions; this.useFetchStreams = !!settings.useFetchStreams; validateIsNotUsedTogether( @@ -159,29 +160,30 @@ export class FirestoreSettingsImpl { settings.experimentalAutoDetectLongPolling ); - const experimentalLongPollingTimeout = this.experimentalLongPollingOptions?.idleHttpRequestTimeoutSeconds; + const experimentalLongPollingTimeout = + this.experimentalLongPollingOptions?.idleHttpRequestTimeoutSeconds; if (experimentalLongPollingTimeout !== undefined) { - if (! Number.isInteger(experimentalLongPollingTimeout)) { + if (!Number.isInteger(experimentalLongPollingTimeout)) { throw new FirestoreError( Code.INVALID_ARGUMENT, `invalid long polling timeout: ` + - `${experimentalLongPollingTimeout} (must be an integer)` + `${experimentalLongPollingTimeout} (must be an integer)` ); } if (experimentalLongPollingTimeout < MIN_LONG_POLLING_TIMEOUT_SECONDS) { throw new FirestoreError( Code.INVALID_ARGUMENT, `invalid long polling timeout: ` + - `${experimentalLongPollingTimeout} ` + - `(minimum allowed value is ${MIN_LONG_POLLING_TIMEOUT_SECONDS})` + `${experimentalLongPollingTimeout} ` + + `(minimum allowed value is ${MIN_LONG_POLLING_TIMEOUT_SECONDS})` ); } if (experimentalLongPollingTimeout > MAX_LONG_POLLING_TIMEOUT_SECONDS) { throw new FirestoreError( Code.INVALID_ARGUMENT, `invalid long polling timeout: ` + - `${experimentalLongPollingTimeout} ` + - `(maximum allowed value is ${MAX_LONG_POLLING_TIMEOUT_SECONDS})` + `${experimentalLongPollingTimeout} ` + + `(maximum allowed value is ${MAX_LONG_POLLING_TIMEOUT_SECONDS})` ); } } @@ -197,7 +199,8 @@ export class FirestoreSettingsImpl { other.experimentalForceLongPolling && this.experimentalAutoDetectLongPolling === other.experimentalAutoDetectLongPolling && - (this.experimentalLongPollingOptions?.idleHttpRequestTimeoutSeconds === other.experimentalLongPollingOptions?.idleHttpRequestTimeoutSeconds) && + this.experimentalLongPollingOptions?.idleHttpRequestTimeoutSeconds === + other.experimentalLongPollingOptions?.idleHttpRequestTimeoutSeconds && this.ignoreUndefinedProperties === other.ignoreUndefinedProperties && this.useFetchStreams === other.useFetchStreams ); diff --git a/packages/firestore/src/platform/browser/webchannel_connection.ts b/packages/firestore/src/platform/browser/webchannel_connection.ts index 977a4e782ab..64c3ef00a8d 100644 --- a/packages/firestore/src/platform/browser/webchannel_connection.ts +++ b/packages/firestore/src/platform/browser/webchannel_connection.ts @@ -57,7 +57,7 @@ export class WebChannelConnection extends RestConnection { private readonly forceLongPolling: boolean; private readonly autoDetectLongPolling: boolean; private readonly useFetchStreams: boolean; - private readonly longPollingTimeout: number | undefined; + private readonly longPollingTimeout: number | null; constructor(info: DatabaseInfo) { super(info); diff --git a/packages/firestore/test/unit/api/database.test.ts b/packages/firestore/test/unit/api/database.test.ts index 76930fcda84..2b32362b8d7 100644 --- a/packages/firestore/test/unit/api/database.test.ts +++ b/packages/firestore/test/unit/api/database.test.ts @@ -265,7 +265,7 @@ describe('SnapshotMetadata', () => { }); }); -describe.only('Settings', () => { +describe('Settings', () => { it('can not use mutually exclusive settings together', () => { // Use a new instance of Firestore in order to configure settings. const db = newTestFirestore(); @@ -282,63 +282,98 @@ describe.only('Settings', () => { it('idleHttpRequestTimeoutSeconds is undefined by default', () => { // Use a new instance of Firestore in order to configure settings. const db = newTestFirestore(); - expect(db._getSettings().experimentalLongPollingOptions?.idleHttpRequestTimeoutSeconds).to.be.undefined; + expect( + db._getSettings().experimentalLongPollingOptions + ?.idleHttpRequestTimeoutSeconds + ).to.be.undefined; }); it('idleHttpRequestTimeoutSeconds minimum value is allowed', () => { // Use a new instance of Firestore in order to configure settings. const db = newTestFirestore(); - db._setSettings({experimentalLongPollingOptions: { idleHttpRequestTimeoutSeconds: 5 }}); - expect(db._getSettings().experimentalLongPollingOptions?.idleHttpRequestTimeoutSeconds).to.equal(5); + db._setSettings({ + experimentalLongPollingOptions: { idleHttpRequestTimeoutSeconds: 5 } + }); + expect( + db._getSettings().experimentalLongPollingOptions + ?.idleHttpRequestTimeoutSeconds + ).to.equal(5); }); it('idleHttpRequestTimeoutSeconds maximum value is allowed', () => { // Use a new instance of Firestore in order to configure settings. const db = newTestFirestore(); - db._setSettings({experimentalLongPollingOptions: { idleHttpRequestTimeoutSeconds: 30 }}); - expect(db._getSettings().experimentalLongPollingOptions?.idleHttpRequestTimeoutSeconds).to.equal(30); + db._setSettings({ + experimentalLongPollingOptions: { idleHttpRequestTimeoutSeconds: 30 } + }); + expect( + db._getSettings().experimentalLongPollingOptions + ?.idleHttpRequestTimeoutSeconds + ).to.equal(30); }); it('idleHttpRequestTimeoutSeconds typical value is allowed', () => { // Use a new instance of Firestore in order to configure settings. const db = newTestFirestore(); - db._setSettings({experimentalLongPollingOptions: { idleHttpRequestTimeoutSeconds: 25 }}); - expect(db._getSettings().experimentalLongPollingOptions?.idleHttpRequestTimeoutSeconds).to.equal(25); + db._setSettings({ + experimentalLongPollingOptions: { idleHttpRequestTimeoutSeconds: 25 } + }); + expect( + db._getSettings().experimentalLongPollingOptions + ?.idleHttpRequestTimeoutSeconds + ).to.equal(25); }); it('idleHttpRequestTimeoutSeconds value one less than minimum throws', () => { // Use a new instance of Firestore in order to configure settings. const db = newTestFirestore(); - expect(() => db._setSettings({experimentalLongPollingOptions: { idleHttpRequestTimeoutSeconds: 4 }})) - .to.throw(/invalid.*timeout.*4.*\(.*5.*\)/i); + expect(() => + db._setSettings({ + experimentalLongPollingOptions: { idleHttpRequestTimeoutSeconds: 4 } + }) + ).to.throw(/invalid.*timeout.*4.*\(.*5.*\)/i); }); it('idleHttpRequestTimeoutSeconds value one more than maximum throws', () => { // Use a new instance of Firestore in order to configure settings. const db = newTestFirestore(); - expect(() => db._setSettings({experimentalLongPollingOptions: { idleHttpRequestTimeoutSeconds: 31 }})) - .to.throw(/invalid.*timeout.*31.*\(.*30.*\)/i); + expect(() => + db._setSettings({ + experimentalLongPollingOptions: { idleHttpRequestTimeoutSeconds: 31 } + }) + ).to.throw(/invalid.*timeout.*31.*\(.*30.*\)/i); }); it('idleHttpRequestTimeoutSeconds value of 0 throws', () => { // Use a new instance of Firestore in order to configure settings. const db = newTestFirestore(); - expect(() => db._setSettings({experimentalLongPollingOptions: { idleHttpRequestTimeoutSeconds: 0 }})) - .to.throw(/invalid.*timeout.*0.*\(.*5.*\)/i); + expect(() => + db._setSettings({ + experimentalLongPollingOptions: { idleHttpRequestTimeoutSeconds: 0 } + }) + ).to.throw(/invalid.*timeout.*0.*\(.*5.*\)/i); }); it('idleHttpRequestTimeoutSeconds value of -1 throws', () => { // Use a new instance of Firestore in order to configure settings. const db = newTestFirestore(); - expect(() => db._setSettings({experimentalLongPollingOptions: { idleHttpRequestTimeoutSeconds: -1 }})) - .to.throw(/invalid.*timeout.*-1.*\(.*5.*\)/i); + expect(() => + db._setSettings({ + experimentalLongPollingOptions: { idleHttpRequestTimeoutSeconds: -1 } + }) + ).to.throw(/invalid.*timeout.*-1.*\(.*5.*\)/i); }); it('idleHttpRequestTimeoutSeconds non-integral value throws', () => { // Use a new instance of Firestore in order to configure settings. const db = newTestFirestore(); - expect(() => db._setSettings({experimentalLongPollingOptions: { idleHttpRequestTimeoutSeconds: 123.456 }})) - .to.throw(/invalid.*timeout.*123.456.*\(.*integer.*\)/i); + expect(() => + db._setSettings({ + experimentalLongPollingOptions: { + idleHttpRequestTimeoutSeconds: 123.456 + } + }) + ).to.throw(/invalid.*timeout.*123.456.*\(.*integer.*\)/i); }); it('gets settings from useEmulator', () => { From 57635d43d6ce04271bae560794809c88925c4e81 Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Fri, 14 Apr 2023 01:27:44 -0400 Subject: [PATCH 05/21] ExperimentalLongPollingOptions interface, part 3 [no ci] --- packages/firestore/src/api.ts | 7 +- .../firestore/src/api/long_polling_options.ts | 69 +++++++++++++++++++ packages/firestore/src/api/settings.ts | 26 +------ packages/firestore/src/core/database_info.ts | 7 +- packages/firestore/src/lite-api/components.ts | 9 +-- packages/firestore/src/lite-api/settings.ts | 22 +++--- .../platform/browser/webchannel_connection.ts | 11 +-- .../test/integration/util/internal_helpers.ts | 8 ++- 8 files changed, 103 insertions(+), 56 deletions(-) create mode 100644 packages/firestore/src/api/long_polling_options.ts diff --git a/packages/firestore/src/api.ts b/packages/firestore/src/api.ts index 082fe37db0b..a5fc62f0c0f 100644 --- a/packages/firestore/src/api.ts +++ b/packages/firestore/src/api.ts @@ -75,11 +75,8 @@ export { TaskState } from './api/bundle'; -export { - ExperimentalLongPollingOptions, - FirestoreSettings, - PersistenceSettings -} from './api/settings'; +export { FirestoreSettings, PersistenceSettings } from './api/settings'; +export { ExperimentalLongPollingOptions } from './api/long_polling_options'; export { DocumentChange, diff --git a/packages/firestore/src/api/long_polling_options.ts b/packages/firestore/src/api/long_polling_options.ts new file mode 100644 index 00000000000..d615a7e8c1a --- /dev/null +++ b/packages/firestore/src/api/long_polling_options.ts @@ -0,0 +1,69 @@ +/** + * @license + * Copyright 2023 Google LLC + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +/** + * Options for long polling when it is used. + * + * See `FirestoreSettings.experimentalAutoDetectLongPolling`, + * `FirestoreSettings.experimentalForceLongPolling`, and + * `FirestoreSettings.experimentalLongPollingOptions`. + */ +export interface ExperimentalLongPollingOptions { + /** + * The desired maximum timeout interval (in seconds) to complete a long + * polling GET response. Valid values are integers between 5 and 30, + * inclusive. + * + * By default, when long polling is used the "hanging GET" request sent by + * the client times out after 30 seconds. To request a different timeout + * from the server, set this setting with the desired timeout. This may be + * useful, for example, if the buffering proxy that necessitated enabling + * long polling in the first place has a shorter timeout for hanging GET + * requests, in which case setting the long-polling timeout to a shorter + * value, such as 25 seconds, may fix prematurely-closed hanging GET + * requests. + */ + idleHttpRequestTimeoutSeconds?: number; +} + +/** + * Compares two `ExperimentalLongPollingOptions` objects for equality. + */ +export function longPollingOptionsEqual( + options1: ExperimentalLongPollingOptions, + options2: ExperimentalLongPollingOptions +): boolean { + return ( + options1?.idleHttpRequestTimeoutSeconds === + options2?.idleHttpRequestTimeoutSeconds + ); +} + +/** + * Creates and returns a new `ExperimentalLongPollingOptions` with the same + * option values as the given instance. + */ +export function cloneLongPollingOptions( + options: ExperimentalLongPollingOptions +): ExperimentalLongPollingOptions { + const result: ExperimentalLongPollingOptions = {}; + if (options.idleHttpRequestTimeoutSeconds !== undefined) { + result.idleHttpRequestTimeoutSeconds = + options.idleHttpRequestTimeoutSeconds; + } + return result; +} diff --git a/packages/firestore/src/api/settings.ts b/packages/firestore/src/api/settings.ts index 9dc9572f85a..c3c2f06b003 100644 --- a/packages/firestore/src/api/settings.ts +++ b/packages/firestore/src/api/settings.ts @@ -18,6 +18,7 @@ import { FirestoreSettings as LiteSettings } from '../lite-api/settings'; import { FirestoreLocalCache } from './cache_config'; +import { ExperimentalLongPollingOptions } from './long_polling_options'; export { DEFAULT_HOST } from '../lite-api/settings'; @@ -102,28 +103,3 @@ export interface FirestoreSettings extends LiteSettings { */ experimentalLongPollingOptions?: ExperimentalLongPollingOptions; } - -/** - * Options for long polling when it is used. - * - * See `FirestoreSettings.experimentalAutoDetectLongPolling`, - * `FirestoreSettings.experimentalForceLongPolling`, and - * `FirestoreSettings.experimentalLongPollingOptions`. - */ -export interface ExperimentalLongPollingOptions { - /** - * The desired maximum timeout interval (in seconds) to complete a long - * polling GET response. Valid values are integers between 5 and 30, - * inclusive. - * - * By default, when long polling is used the "hanging GET" request sent by - * the client times out after 30 seconds. To request a different timeout - * from the server, set this setting with the desired timeout. This may be - * useful, for example, if the buffering proxy that necessitated enabling - * long polling in the first place has a shorter timeout for hanging GET - * requests, in which case setting the long-polling timeout to a shorter - * value, such as 25 seconds, may fix prematurely-closed hanging GET - * requests. - */ - idleHttpRequestTimeoutSeconds?: number; -} diff --git a/packages/firestore/src/core/database_info.ts b/packages/firestore/src/core/database_info.ts index 766225f6a59..4d3d2fe866b 100644 --- a/packages/firestore/src/core/database_info.ts +++ b/packages/firestore/src/core/database_info.ts @@ -1,5 +1,6 @@ import { FirebaseApp } from '@firebase/app'; +import { ExperimentalLongPollingOptions } from '../api/long_polling_options'; import { Code, FirestoreError } from '../util/error'; /** @@ -34,9 +35,7 @@ export class DatabaseInfo { * when using WebChannel as the network transport. * @param autoDetectLongPolling - Whether to use the detectBufferingProxy * option when using WebChannel as the network transport. - * @param longPollingTimeout The desired maximum timeout interval (in - * milliseconds) to complete a long-polling GET response; the server _may_ - * ignore this value; may be null to use the default (30 seconds). + * @param longPollingOptions Options that configure long polling, if used. * @param useFetchStreams Whether to use the Fetch API instead of * XMLHTTPRequest */ @@ -48,7 +47,7 @@ export class DatabaseInfo { readonly ssl: boolean, readonly forceLongPolling: boolean, readonly autoDetectLongPolling: boolean, - readonly longPollingTimeout: number | null, + readonly longPollingOptions: ExperimentalLongPollingOptions, readonly useFetchStreams: boolean ) {} } diff --git a/packages/firestore/src/lite-api/components.ts b/packages/firestore/src/lite-api/components.ts index e11caa63394..436d2b5d4d8 100644 --- a/packages/firestore/src/lite-api/components.ts +++ b/packages/firestore/src/lite-api/components.ts @@ -19,6 +19,7 @@ import { _FirebaseService } from '@firebase/app'; import { CredentialsProvider } from '../api/credentials'; +import { cloneLongPollingOptions } from '../api/long_polling_options'; import { User } from '../auth/user'; import { DatabaseId, DatabaseInfo } from '../core/database_info'; import { newConnection } from '../platform/connection'; @@ -117,13 +118,7 @@ export function makeDatabaseInfo( settings.ssl, settings.experimentalForceLongPolling, settings.experimentalAutoDetectLongPolling, - getLongPollingTimeout(settings), + cloneLongPollingOptions(settings.experimentalLongPollingOptions), settings.useFetchStreams ); } - -function getLongPollingTimeout(settings: FirestoreSettingsImpl): number | null { - const value = - settings.experimentalLongPollingOptions?.idleHttpRequestTimeoutSeconds; - return value === undefined ? null : value * 1000; -} diff --git a/packages/firestore/src/lite-api/settings.ts b/packages/firestore/src/lite-api/settings.ts index 9098c6e22ea..fa1851c170c 100644 --- a/packages/firestore/src/lite-api/settings.ts +++ b/packages/firestore/src/lite-api/settings.ts @@ -17,6 +17,10 @@ import { FirestoreLocalCache } from '../api/cache_config'; import { CredentialsSettings } from '../api/credentials'; +import { + ExperimentalLongPollingOptions, + longPollingOptionsEqual +} from '../api/long_polling_options'; import { LRU_COLLECTION_DISABLED, LRU_DEFAULT_CACHE_SIZE_BYTES @@ -71,9 +75,7 @@ export interface PrivateSettings extends FirestoreSettings { // Used in firestore@exp experimentalAutoDetectLongPolling?: boolean; // Used in firestore@exp - experimentalLongPollingOptions?: { - idleHttpRequestTimeoutSeconds?: number; - }; + experimentalLongPollingOptions?: ExperimentalLongPollingOptions; // Used in firestore@exp useFetchStreams?: boolean; @@ -98,9 +100,7 @@ export class FirestoreSettingsImpl { readonly experimentalAutoDetectLongPolling: boolean; - readonly experimentalLongPollingOptions?: { - idleHttpRequestTimeoutSeconds?: number; - }; + readonly experimentalLongPollingOptions: ExperimentalLongPollingOptions; readonly ignoreUndefinedProperties: boolean; @@ -150,7 +150,7 @@ export class FirestoreSettingsImpl { this.experimentalAutoDetectLongPolling = !!settings.experimentalAutoDetectLongPolling; this.experimentalLongPollingOptions = - settings.experimentalLongPollingOptions; + settings.experimentalLongPollingOptions ?? {}; this.useFetchStreams = !!settings.useFetchStreams; validateIsNotUsedTogether( @@ -161,7 +161,7 @@ export class FirestoreSettingsImpl { ); const experimentalLongPollingTimeout = - this.experimentalLongPollingOptions?.idleHttpRequestTimeoutSeconds; + this.experimentalLongPollingOptions.idleHttpRequestTimeoutSeconds; if (experimentalLongPollingTimeout !== undefined) { if (!Number.isInteger(experimentalLongPollingTimeout)) { throw new FirestoreError( @@ -199,8 +199,10 @@ export class FirestoreSettingsImpl { other.experimentalForceLongPolling && this.experimentalAutoDetectLongPolling === other.experimentalAutoDetectLongPolling && - this.experimentalLongPollingOptions?.idleHttpRequestTimeoutSeconds === - other.experimentalLongPollingOptions?.idleHttpRequestTimeoutSeconds && + longPollingOptionsEqual( + this.experimentalLongPollingOptions, + other.experimentalLongPollingOptions + ) && this.ignoreUndefinedProperties === other.ignoreUndefinedProperties && this.useFetchStreams === other.useFetchStreams ); diff --git a/packages/firestore/src/platform/browser/webchannel_connection.ts b/packages/firestore/src/platform/browser/webchannel_connection.ts index 64c3ef00a8d..49869be96f9 100644 --- a/packages/firestore/src/platform/browser/webchannel_connection.ts +++ b/packages/firestore/src/platform/browser/webchannel_connection.ts @@ -32,6 +32,7 @@ import { } from '@firebase/webchannel-wrapper'; import { Token } from '../../api/credentials'; +import { ExperimentalLongPollingOptions } from '../../api/long_polling_options'; import { DatabaseInfo } from '../../core/database_info'; import { Stream } from '../../remote/connection'; import { RestConnection } from '../../remote/rest_connection'; @@ -57,14 +58,14 @@ export class WebChannelConnection extends RestConnection { private readonly forceLongPolling: boolean; private readonly autoDetectLongPolling: boolean; private readonly useFetchStreams: boolean; - private readonly longPollingTimeout: number | null; + private readonly longPollingOptions: ExperimentalLongPollingOptions; constructor(info: DatabaseInfo) { super(info); this.forceLongPolling = info.forceLongPolling; this.autoDetectLongPolling = info.autoDetectLongPolling; this.useFetchStreams = info.useFetchStreams; - this.longPollingTimeout = info.longPollingTimeout; + this.longPollingOptions = info.longPollingOptions; } protected performRPCRequest( @@ -202,8 +203,10 @@ export class WebChannelConnection extends RestConnection { detectBufferingProxy: this.autoDetectLongPolling }; - if (this.longPollingTimeout) { - request.longPollingTimeout = this.longPollingTimeout; + const idleHttpRequestTimeoutSeconds = + this.longPollingOptions.idleHttpRequestTimeoutSeconds; + if (idleHttpRequestTimeoutSeconds !== undefined) { + request.longPollingTimeout = idleHttpRequestTimeoutSeconds * 1000; } if (this.useFetchStreams) { diff --git a/packages/firestore/test/integration/util/internal_helpers.ts b/packages/firestore/test/integration/util/internal_helpers.ts index dfcaedc47d6..8000211ed8a 100644 --- a/packages/firestore/test/integration/util/internal_helpers.ts +++ b/packages/firestore/test/integration/util/internal_helpers.ts @@ -21,6 +21,10 @@ import { EmptyAppCheckTokenProvider, EmptyAuthCredentialsProvider } from '../../../src/api/credentials'; +import { + ExperimentalLongPollingOptions, + cloneLongPollingOptions +} from '../../../src/api/long_polling_options'; import { User } from '../../../src/auth/user'; import { DatabaseId, DatabaseInfo } from '../../../src/core/database_info'; import { newConnection } from '../../../src/platform/connection'; @@ -57,7 +61,9 @@ export function getDefaultDatabaseInfo(): DatabaseInfo { !!DEFAULT_SETTINGS.ssl, !!DEFAULT_SETTINGS.experimentalForceLongPolling, !!DEFAULT_SETTINGS.experimentalAutoDetectLongPolling, - DEFAULT_SETTINGS.experimentalLongPollingOptions, + cloneLongPollingOptions( + DEFAULT_SETTINGS.experimentalLongPollingOptions ?? {} + ), /*use FetchStreams= */ false ); } From 8e892cacf56012d36ab7b98e0bb3e1a3f3f317bb Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Fri, 14 Apr 2023 01:34:07 -0400 Subject: [PATCH 06/21] ExperimentalLongPollingOptions interface, part 4 (done) --- common/api-review/firestore.api.md | 9 ++++++--- .../firestore/test/unit/remote/rest_connection.test.ts | 2 +- packages/firestore/test/unit/specs/spec_test_runner.ts | 2 +- 3 files changed, 8 insertions(+), 5 deletions(-) diff --git a/common/api-review/firestore.api.md b/common/api-review/firestore.api.md index 97dcc46ec87..da1e8461c8e 100644 --- a/common/api-review/firestore.api.md +++ b/common/api-review/firestore.api.md @@ -184,6 +184,11 @@ export function endBefore(snapshot: DocumentSnapshot): QueryEndAtConstr // @public export function endBefore(...fieldValues: unknown[]): QueryEndAtConstraint; +// @public +export interface ExperimentalLongPollingOptions { + idleHttpRequestTimeoutSeconds?: number; +} + // @public export class FieldPath { constructor(...fieldNames: string[]); @@ -227,9 +232,7 @@ export interface FirestoreSettings { cacheSizeBytes?: number; experimentalAutoDetectLongPolling?: boolean; experimentalForceLongPolling?: boolean; - experimentalLongPollingOptions?: { - idleHttpRequestTimeoutSeconds?: number; - }; + experimentalLongPollingOptions?: ExperimentalLongPollingOptions; host?: string; ignoreUndefinedProperties?: boolean; localCache?: FirestoreLocalCache; diff --git a/packages/firestore/test/unit/remote/rest_connection.test.ts b/packages/firestore/test/unit/remote/rest_connection.test.ts index 0f89d23928f..838e9447660 100644 --- a/packages/firestore/test/unit/remote/rest_connection.test.ts +++ b/packages/firestore/test/unit/remote/rest_connection.test.ts @@ -65,7 +65,7 @@ describe('RestConnection', () => { /*ssl=*/ false, /*forceLongPolling=*/ false, /*autoDetectLongPolling=*/ false, - /*longPollingTimeout=*/ undefined, + /*longPollingOptions=*/ {}, /*useFetchStreams=*/ false ); const connection = new TestRestConnection(testDatabaseInfo); diff --git a/packages/firestore/test/unit/specs/spec_test_runner.ts b/packages/firestore/test/unit/specs/spec_test_runner.ts index 072ca41e04e..a0744929342 100644 --- a/packages/firestore/test/unit/specs/spec_test_runner.ts +++ b/packages/firestore/test/unit/specs/spec_test_runner.ts @@ -273,7 +273,7 @@ abstract class TestRunner { /*ssl=*/ false, /*forceLongPolling=*/ false, /*autoDetectLongPolling=*/ false, - /*longPollingTimeout=*/ undefined, + /*longPollingOptions=*/ {}, /*useFetchStreams=*/ false ); From d8966ea923a0abf702338b76f97cabc615c7faf6 Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Fri, 14 Apr 2023 10:02:56 -0400 Subject: [PATCH 07/21] yarn docgen devsite --- ...restore_.experimentallongpollingoptions.md | 39 +++++++++++++++++++ docs-devsite/firestore_.firestoresettings.md | 13 +++++++ docs-devsite/firestore_.md | 1 + 3 files changed, 53 insertions(+) create mode 100644 docs-devsite/firestore_.experimentallongpollingoptions.md diff --git a/docs-devsite/firestore_.experimentallongpollingoptions.md b/docs-devsite/firestore_.experimentallongpollingoptions.md new file mode 100644 index 00000000000..a37bb87e337 --- /dev/null +++ b/docs-devsite/firestore_.experimentallongpollingoptions.md @@ -0,0 +1,39 @@ +Project: /docs/reference/js/_project.yaml +Book: /docs/reference/_book.yaml +page_type: reference + +{% comment %} +DO NOT EDIT THIS FILE! +This is generated by the JS SDK team, and any local changes will be +overwritten. Changes should be made in the source code at +https://github.com/firebase/firebase-js-sdk +{% endcomment %} + +# ExperimentalLongPollingOptions interface +Options for long polling when it is used. + +See `FirestoreSettings.experimentalAutoDetectLongPolling`, `FirestoreSettings.experimentalForceLongPolling`, and `FirestoreSettings.experimentalLongPollingOptions`. + +Signature: + +```typescript +export declare interface ExperimentalLongPollingOptions +``` + +## Properties + +| Property | Type | Description | +| --- | --- | --- | +| [idleHttpRequestTimeoutSeconds](./firestore_.experimentallongpollingoptions.md#experimentallongpollingoptionsidlehttprequesttimeoutseconds) | number | The desired maximum timeout interval (in seconds) to complete a long polling GET response. Valid values are integers between 5 and 30, inclusive.By default, when long polling is used the "hanging GET" request sent by the client times out after 30 seconds. To request a different timeout from the server, set this setting with the desired timeout. This may be useful, for example, if the buffering proxy that necessitated enabling long polling in the first place has a shorter timeout for hanging GET requests, in which case setting the long-polling timeout to a shorter value, such as 25 seconds, may fix prematurely-closed hanging GET requests. | + +## ExperimentalLongPollingOptions.idleHttpRequestTimeoutSeconds + +The desired maximum timeout interval (in seconds) to complete a long polling GET response. Valid values are integers between 5 and 30, inclusive. + +By default, when long polling is used the "hanging GET" request sent by the client times out after 30 seconds. To request a different timeout from the server, set this setting with the desired timeout. This may be useful, for example, if the buffering proxy that necessitated enabling long polling in the first place has a shorter timeout for hanging GET requests, in which case setting the long-polling timeout to a shorter value, such as 25 seconds, may fix prematurely-closed hanging GET requests. + +Signature: + +```typescript +idleHttpRequestTimeoutSeconds?: number; +``` diff --git a/docs-devsite/firestore_.firestoresettings.md b/docs-devsite/firestore_.firestoresettings.md index e4d4846ee64..85827dcd2de 100644 --- a/docs-devsite/firestore_.firestoresettings.md +++ b/docs-devsite/firestore_.firestoresettings.md @@ -25,6 +25,7 @@ export declare interface FirestoreSettings | [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. | | [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. | +| [experimentalLongPollingOptions](./firestore_.firestoresettings.md#firestoresettingsexperimentallongpollingoptions) | [ExperimentalLongPollingOptions](./firestore_.experimentallongpollingoptions.md#experimentallongpollingoptions_interface) | Options for long polling when it is used.These options are only used if experimentalForceLongPolling is true or if experimentalAutoDetectLongPolling is true and the auto-detection determined that long-polling was needed. Otherwise, they have no effect. | | [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. | | [localCache](./firestore_.firestoresettings.md#firestoresettingslocalcache) | [FirestoreLocalCache](./firestore_.md#firestorelocalcache) | Specifies the cache used by the SDK. Available options are MemoryLocalCache and IndexedDbLocalCache, each with different configuration options.When unspecified, MemoryLocalCache will be used by default.NOTE: setting this field and cacheSizeBytes at the same time will throw exception during SDK initialization. Instead, using the configuration in the FirestoreLocalCache object to specify the cache size. | @@ -68,6 +69,18 @@ This setting cannot be used with `experimentalAutoDetectLongPolling` and may be experimentalForceLongPolling?: boolean; ``` +## FirestoreSettings.experimentalLongPollingOptions + +Options for long polling when it is used. + +These options are only used if `experimentalForceLongPolling` is true or if `experimentalAutoDetectLongPolling` is true and the auto-detection determined that long-polling was needed. Otherwise, they have no effect. + +Signature: + +```typescript +experimentalLongPollingOptions?: ExperimentalLongPollingOptions; +``` + ## FirestoreSettings.host The hostname to connect to. diff --git a/docs-devsite/firestore_.md b/docs-devsite/firestore_.md index 7088266cd00..51f7e294e8c 100644 --- a/docs-devsite/firestore_.md +++ b/docs-devsite/firestore_.md @@ -147,6 +147,7 @@ https://github.com/firebase/firebase-js-sdk | [AggregateSpec](./firestore_.aggregatespec.md#aggregatespec_interface) | Specifies a set of aggregations and their aliases. | | [DocumentChange](./firestore_.documentchange.md#documentchange_interface) | A DocumentChange represents a change to the documents matching a query. It contains the document affected and the type of change that occurred. | | [DocumentData](./firestore_.documentdata.md#documentdata_interface) | Document data (for use with [setDoc()](./firestore_lite.md#setdoc)) consists of fields mapped to values. | +| [ExperimentalLongPollingOptions](./firestore_.experimentallongpollingoptions.md#experimentallongpollingoptions_interface) | Options for long polling when it is used.See FirestoreSettings.experimentalAutoDetectLongPolling, FirestoreSettings.experimentalForceLongPolling, and FirestoreSettings.experimentalLongPollingOptions. | | [FirestoreDataConverter](./firestore_.firestoredataconverter.md#firestoredataconverter_interface) | Converter used by withConverter() to transform user objects of type T into Firestore data.Using the converter allows you to specify generic type arguments when storing and retrieving objects from Firestore. | | [FirestoreSettings](./firestore_.firestoresettings.md#firestoresettings_interface) | Specifies custom configurations for your Cloud Firestore instance. You must set these before invoking any other methods. | | [Index](./firestore_.index.md#index_interface) | (BETA) The SDK definition of a Firestore index. | From 9296b487132a1b2b643fe3a3b50d5f05bb09d38b Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Fri, 14 Apr 2023 10:04:04 -0400 Subject: [PATCH 08/21] internal_helpers.ts: remove unused import: ExperimentalLongPollingOptions --- packages/firestore/test/integration/util/internal_helpers.ts | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/packages/firestore/test/integration/util/internal_helpers.ts b/packages/firestore/test/integration/util/internal_helpers.ts index 8000211ed8a..4abfcf78096 100644 --- a/packages/firestore/test/integration/util/internal_helpers.ts +++ b/packages/firestore/test/integration/util/internal_helpers.ts @@ -21,10 +21,7 @@ import { EmptyAppCheckTokenProvider, EmptyAuthCredentialsProvider } from '../../../src/api/credentials'; -import { - ExperimentalLongPollingOptions, - cloneLongPollingOptions -} from '../../../src/api/long_polling_options'; +import { cloneLongPollingOptions } from '../../../src/api/long_polling_options'; import { User } from '../../../src/auth/user'; import { DatabaseId, DatabaseInfo } from '../../../src/core/database_info'; import { newConnection } from '../../../src/platform/connection'; From 9e512228405f2e871c1c48bf65fb09cbaa6796e7 Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Fri, 14 Apr 2023 10:05:13 -0400 Subject: [PATCH 09/21] [no ci] From d4ffeec84c32004418a693dbeb9beb0561ab6654 Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Fri, 14 Apr 2023 10:23:13 -0400 Subject: [PATCH 10/21] add unit tests for longPollingOptionsEqual() and cloneLongPollingOptions() --- .../unit/api/long_polling_options.test.ts | 82 +++++++++++++++++++ 1 file changed, 82 insertions(+) create mode 100644 packages/firestore/test/unit/api/long_polling_options.test.ts diff --git a/packages/firestore/test/unit/api/long_polling_options.test.ts b/packages/firestore/test/unit/api/long_polling_options.test.ts new file mode 100644 index 00000000000..ee429293e4d --- /dev/null +++ b/packages/firestore/test/unit/api/long_polling_options.test.ts @@ -0,0 +1,82 @@ +/** + * @license + * Copyright 2023 Google LLC + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +import { expect } from 'chai'; + +import { + ExperimentalLongPollingOptions, + longPollingOptionsEqual, + cloneLongPollingOptions +} from '../../../src/api/long_polling_options'; + +describe('long_polling_options', () => { + it('longPollingOptionsEqual() should return true for empty objects', () => { + expect(longPollingOptionsEqual({}, {})).to.be.true; + }); + + it('longPollingOptionsEqual() should return true if both objects have the same idleHttpRequestTimeoutSeconds', () => { + const options1: ExperimentalLongPollingOptions = { + idleHttpRequestTimeoutSeconds: 123 + }; + const options2: ExperimentalLongPollingOptions = { + idleHttpRequestTimeoutSeconds: 123 + }; + expect(longPollingOptionsEqual(options1, options2)).to.be.true; + }); + + it('longPollingOptionsEqual() should return false if the objects have different idleHttpRequestTimeoutSeconds', () => { + const options1: ExperimentalLongPollingOptions = { + idleHttpRequestTimeoutSeconds: 123 + }; + const options2: ExperimentalLongPollingOptions = { + idleHttpRequestTimeoutSeconds: 321 + }; + expect(longPollingOptionsEqual(options1, options2)).to.be.false; + }); + + it('longPollingOptionsEqual() should ignore properties not defined in ExperimentalLongPollingOptions', () => { + const options1 = { + idleHttpRequestTimeoutSeconds: 123, + someOtherProperty: 42 + } as ExperimentalLongPollingOptions; + const options2 = { + idleHttpRequestTimeoutSeconds: 123, + someOtherProperty: 24 + } as ExperimentalLongPollingOptions; + expect(longPollingOptionsEqual(options1, options2)).to.be.true; + }); + + it('cloneLongPollingOptions() with an empty object should return an empty object', () => { + expect(cloneLongPollingOptions({})).to.deep.equal({}); + }); + + it('cloneLongPollingOptions() should copy idleHttpRequestTimeoutSeconds', () => { + expect( + cloneLongPollingOptions({ idleHttpRequestTimeoutSeconds: 1234 }) + ).to.deep.equal({ idleHttpRequestTimeoutSeconds: 1234 }); + }); + + it('cloneLongPollingOptions() should not copy properties not defined in ExperimentalLongPollingOptions', () => { + const options = { + idleHttpRequestTimeoutSeconds: 1234, + someOtherProperty: 42 + } as ExperimentalLongPollingOptions; + expect(cloneLongPollingOptions(options)).to.deep.equal({ + idleHttpRequestTimeoutSeconds: 1234 + }); + }); +}); From 376dde70483d7697a8b318c4d28e0c7c4e766bdf Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Fri, 14 Apr 2023 10:23:33 -0400 Subject: [PATCH 11/21] long_polling_options.ts: doc cleanup and minor code cleanup --- .../firestore/src/api/long_polling_options.ts | 33 +++++++++++-------- 1 file changed, 19 insertions(+), 14 deletions(-) diff --git a/packages/firestore/src/api/long_polling_options.ts b/packages/firestore/src/api/long_polling_options.ts index d615a7e8c1a..acecf0878bd 100644 --- a/packages/firestore/src/api/long_polling_options.ts +++ b/packages/firestore/src/api/long_polling_options.ts @@ -16,7 +16,9 @@ */ /** - * Options for long polling when it is used. + * Options that configure the "long polling" networking mode. + * + * Note: This interface is "experimental" and is subject to change. * * See `FirestoreSettings.experimentalAutoDetectLongPolling`, * `FirestoreSettings.experimentalForceLongPolling`, and @@ -24,18 +26,20 @@ */ export interface ExperimentalLongPollingOptions { /** - * The desired maximum timeout interval (in seconds) to complete a long + * The desired maximum timeout interval, in seconds, to complete a long * polling GET response. Valid values are integers between 5 and 30, * inclusive. * * By default, when long polling is used the "hanging GET" request sent by * the client times out after 30 seconds. To request a different timeout - * from the server, set this setting with the desired timeout. This may be - * useful, for example, if the buffering proxy that necessitated enabling - * long polling in the first place has a shorter timeout for hanging GET - * requests, in which case setting the long-polling timeout to a shorter - * value, such as 25 seconds, may fix prematurely-closed hanging GET - * requests. + * from the server, set this setting with the desired timeout. + * + * Changing the default timeout may be useful, for example, if the buffering + * proxy that necessitated enabling long polling in the first place has a + * shorter timeout for hanging GET requests, in which case setting the + * long-polling timeout to a shorter value, such as 25 seconds, may fix + * prematurely-closed hanging GET requests. + * For example, see https://github.com/firebase/firebase-js-sdk/issues/6987. */ idleHttpRequestTimeoutSeconds?: number; } @@ -48,8 +52,8 @@ export function longPollingOptionsEqual( options2: ExperimentalLongPollingOptions ): boolean { return ( - options1?.idleHttpRequestTimeoutSeconds === - options2?.idleHttpRequestTimeoutSeconds + options1.idleHttpRequestTimeoutSeconds === + options2.idleHttpRequestTimeoutSeconds ); } @@ -60,10 +64,11 @@ export function longPollingOptionsEqual( export function cloneLongPollingOptions( options: ExperimentalLongPollingOptions ): ExperimentalLongPollingOptions { - const result: ExperimentalLongPollingOptions = {}; + const clone: ExperimentalLongPollingOptions = {}; + if (options.idleHttpRequestTimeoutSeconds !== undefined) { - result.idleHttpRequestTimeoutSeconds = - options.idleHttpRequestTimeoutSeconds; + clone.idleHttpRequestTimeoutSeconds = options.idleHttpRequestTimeoutSeconds; } - return result; + + return clone; } From 7cc521f49db428358a342fb2d50fd7345e96f54b Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Fri, 14 Apr 2023 10:28:52 -0400 Subject: [PATCH 12/21] doc improvements --- packages/firestore/src/api/long_polling_options.ts | 11 ++++++----- packages/firestore/src/api/settings.ts | 6 ++++-- packages/firestore/src/core/database_info.ts | 2 +- 3 files changed, 11 insertions(+), 8 deletions(-) diff --git a/packages/firestore/src/api/long_polling_options.ts b/packages/firestore/src/api/long_polling_options.ts index acecf0878bd..7ec1a915521 100644 --- a/packages/firestore/src/api/long_polling_options.ts +++ b/packages/firestore/src/api/long_polling_options.ts @@ -16,7 +16,8 @@ */ /** - * Options that configure the "long polling" networking mode. + * Options that configure the SDK’s underlying network transport (WebChannel) + * when long-polling is used. * * Note: This interface is "experimental" and is subject to change. * @@ -26,16 +27,16 @@ */ export interface ExperimentalLongPollingOptions { /** - * The desired maximum timeout interval, in seconds, to complete a long - * polling GET response. Valid values are integers between 5 and 30, + * The desired maximum timeout interval, in seconds, to complete a + * long-polling GET response. Valid values are integers between 5 and 30, * inclusive. * - * By default, when long polling is used the "hanging GET" request sent by + * By default, when long-polling is used the "hanging GET" request sent by * the client times out after 30 seconds. To request a different timeout * from the server, set this setting with the desired timeout. * * Changing the default timeout may be useful, for example, if the buffering - * proxy that necessitated enabling long polling in the first place has a + * proxy that necessitated enabling long-polling in the first place has a * shorter timeout for hanging GET requests, in which case setting the * long-polling timeout to a shorter value, such as 25 seconds, may fix * prematurely-closed hanging GET requests. diff --git a/packages/firestore/src/api/settings.ts b/packages/firestore/src/api/settings.ts index c3c2f06b003..babf9499387 100644 --- a/packages/firestore/src/api/settings.ts +++ b/packages/firestore/src/api/settings.ts @@ -95,11 +95,13 @@ export interface FirestoreSettings extends LiteSettings { experimentalAutoDetectLongPolling?: boolean; /** - * Options for long polling when it is used. + * Options that configure the SDK’s underlying network transport (WebChannel) + * when long-polling is used. * * These options are only used if `experimentalForceLongPolling` is true or if * `experimentalAutoDetectLongPolling` is true and the auto-detection - * determined that long-polling was needed. Otherwise, they have no effect. + * determined that long-polling was needed. Otherwise, these options have no + * effect. */ experimentalLongPollingOptions?: ExperimentalLongPollingOptions; } diff --git a/packages/firestore/src/core/database_info.ts b/packages/firestore/src/core/database_info.ts index 4d3d2fe866b..0325f8166b6 100644 --- a/packages/firestore/src/core/database_info.ts +++ b/packages/firestore/src/core/database_info.ts @@ -35,7 +35,7 @@ export class DatabaseInfo { * when using WebChannel as the network transport. * @param autoDetectLongPolling - Whether to use the detectBufferingProxy * option when using WebChannel as the network transport. - * @param longPollingOptions Options that configure long polling, if used. + * @param longPollingOptions Options that configure long-polling. * @param useFetchStreams Whether to use the Fetch API instead of * XMLHTTPRequest */ From 23685ba7c017ef8e6b098932e9dd8423f7ce6135 Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Fri, 14 Apr 2023 10:46:26 -0400 Subject: [PATCH 13/21] lite-api/settings.ts: move validation logic to a helper method, validateLongPollingOptions() --- packages/firestore/src/lite-api/settings.ts | 68 ++++++++++++--------- 1 file changed, 39 insertions(+), 29 deletions(-) diff --git a/packages/firestore/src/lite-api/settings.ts b/packages/firestore/src/lite-api/settings.ts index fa1851c170c..392c078ae53 100644 --- a/packages/firestore/src/lite-api/settings.ts +++ b/packages/firestore/src/lite-api/settings.ts @@ -19,6 +19,7 @@ import { FirestoreLocalCache } from '../api/cache_config'; import { CredentialsSettings } from '../api/credentials'; import { ExperimentalLongPollingOptions, + cloneLongPollingOptions, longPollingOptionsEqual } from '../api/long_polling_options'; import { @@ -149,8 +150,9 @@ export class FirestoreSettingsImpl { this.experimentalForceLongPolling = !!settings.experimentalForceLongPolling; this.experimentalAutoDetectLongPolling = !!settings.experimentalAutoDetectLongPolling; - this.experimentalLongPollingOptions = - settings.experimentalLongPollingOptions ?? {}; + this.experimentalLongPollingOptions = cloneLongPollingOptions( + settings.experimentalLongPollingOptions ?? {} + ); this.useFetchStreams = !!settings.useFetchStreams; validateIsNotUsedTogether( @@ -160,33 +162,7 @@ export class FirestoreSettingsImpl { settings.experimentalAutoDetectLongPolling ); - const experimentalLongPollingTimeout = - this.experimentalLongPollingOptions.idleHttpRequestTimeoutSeconds; - if (experimentalLongPollingTimeout !== undefined) { - if (!Number.isInteger(experimentalLongPollingTimeout)) { - throw new FirestoreError( - Code.INVALID_ARGUMENT, - `invalid long polling timeout: ` + - `${experimentalLongPollingTimeout} (must be an integer)` - ); - } - if (experimentalLongPollingTimeout < MIN_LONG_POLLING_TIMEOUT_SECONDS) { - throw new FirestoreError( - Code.INVALID_ARGUMENT, - `invalid long polling timeout: ` + - `${experimentalLongPollingTimeout} ` + - `(minimum allowed value is ${MIN_LONG_POLLING_TIMEOUT_SECONDS})` - ); - } - if (experimentalLongPollingTimeout > MAX_LONG_POLLING_TIMEOUT_SECONDS) { - throw new FirestoreError( - Code.INVALID_ARGUMENT, - `invalid long polling timeout: ` + - `${experimentalLongPollingTimeout} ` + - `(maximum allowed value is ${MAX_LONG_POLLING_TIMEOUT_SECONDS})` - ); - } - } + validateLongPollingOptions(this.experimentalLongPollingOptions); } isEqual(other: FirestoreSettingsImpl): boolean { @@ -208,3 +184,37 @@ export class FirestoreSettingsImpl { ); } } + +function validateLongPollingOptions( + options: ExperimentalLongPollingOptions +): void { + if (options.idleHttpRequestTimeoutSeconds !== undefined) { + if (!Number.isInteger(options.idleHttpRequestTimeoutSeconds)) { + throw new FirestoreError( + Code.INVALID_ARGUMENT, + `invalid long polling timeout: ` + + `${options.idleHttpRequestTimeoutSeconds} (must be an integer)` + ); + } + if ( + options.idleHttpRequestTimeoutSeconds < MIN_LONG_POLLING_TIMEOUT_SECONDS + ) { + throw new FirestoreError( + Code.INVALID_ARGUMENT, + `invalid long polling timeout: ` + + `${options.idleHttpRequestTimeoutSeconds} ` + + `(minimum allowed value is ${MIN_LONG_POLLING_TIMEOUT_SECONDS})` + ); + } + if ( + options.idleHttpRequestTimeoutSeconds > MAX_LONG_POLLING_TIMEOUT_SECONDS + ) { + throw new FirestoreError( + Code.INVALID_ARGUMENT, + `invalid long polling timeout: ` + + `${options.idleHttpRequestTimeoutSeconds} ` + + `(maximum allowed value is ${MAX_LONG_POLLING_TIMEOUT_SECONDS})` + ); + } + } +} From 061d11bcf3bf67eded5bc99e10e56c425c2b55ec Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Fri, 14 Apr 2023 10:50:24 -0400 Subject: [PATCH 14/21] database.test.ts: ?. -> . --- 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 2b32362b8d7..85377c7125a 100644 --- a/packages/firestore/test/unit/api/database.test.ts +++ b/packages/firestore/test/unit/api/database.test.ts @@ -284,7 +284,7 @@ describe('Settings', () => { const db = newTestFirestore(); expect( db._getSettings().experimentalLongPollingOptions - ?.idleHttpRequestTimeoutSeconds + .idleHttpRequestTimeoutSeconds ).to.be.undefined; }); @@ -296,7 +296,7 @@ describe('Settings', () => { }); expect( db._getSettings().experimentalLongPollingOptions - ?.idleHttpRequestTimeoutSeconds + .idleHttpRequestTimeoutSeconds ).to.equal(5); }); @@ -308,7 +308,7 @@ describe('Settings', () => { }); expect( db._getSettings().experimentalLongPollingOptions - ?.idleHttpRequestTimeoutSeconds + .idleHttpRequestTimeoutSeconds ).to.equal(30); }); @@ -320,7 +320,7 @@ describe('Settings', () => { }); expect( db._getSettings().experimentalLongPollingOptions - ?.idleHttpRequestTimeoutSeconds + .idleHttpRequestTimeoutSeconds ).to.equal(25); }); From 1bebedabd994a26cfdcde19afd1b1b1edf206587 Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Fri, 14 Apr 2023 11:19:40 -0400 Subject: [PATCH 15/21] yarn docgen devsite --- .../firestore_.experimentallongpollingoptions.md | 12 ++++++++---- docs-devsite/firestore_.firestoresettings.md | 6 +++--- docs-devsite/firestore_.md | 2 +- 3 files changed, 12 insertions(+), 8 deletions(-) diff --git a/docs-devsite/firestore_.experimentallongpollingoptions.md b/docs-devsite/firestore_.experimentallongpollingoptions.md index a37bb87e337..6ec7a8c90c1 100644 --- a/docs-devsite/firestore_.experimentallongpollingoptions.md +++ b/docs-devsite/firestore_.experimentallongpollingoptions.md @@ -10,7 +10,9 @@ https://github.com/firebase/firebase-js-sdk {% endcomment %} # ExperimentalLongPollingOptions interface -Options for long polling when it is used. +Options that configure the SDK’s underlying network transport (WebChannel) when long-polling is used. + +Note: This interface is "experimental" and is subject to change. See `FirestoreSettings.experimentalAutoDetectLongPolling`, `FirestoreSettings.experimentalForceLongPolling`, and `FirestoreSettings.experimentalLongPollingOptions`. @@ -24,13 +26,15 @@ export declare interface ExperimentalLongPollingOptions | Property | Type | Description | | --- | --- | --- | -| [idleHttpRequestTimeoutSeconds](./firestore_.experimentallongpollingoptions.md#experimentallongpollingoptionsidlehttprequesttimeoutseconds) | number | The desired maximum timeout interval (in seconds) to complete a long polling GET response. Valid values are integers between 5 and 30, inclusive.By default, when long polling is used the "hanging GET" request sent by the client times out after 30 seconds. To request a different timeout from the server, set this setting with the desired timeout. This may be useful, for example, if the buffering proxy that necessitated enabling long polling in the first place has a shorter timeout for hanging GET requests, in which case setting the long-polling timeout to a shorter value, such as 25 seconds, may fix prematurely-closed hanging GET requests. | +| [idleHttpRequestTimeoutSeconds](./firestore_.experimentallongpollingoptions.md#experimentallongpollingoptionsidlehttprequesttimeoutseconds) | number | The desired maximum timeout interval, in seconds, to complete a long-polling GET response. Valid values are integers between 5 and 30, inclusive.By default, when long-polling is used the "hanging GET" request sent by the client times out after 30 seconds. To request a different timeout from the server, set this setting with the desired timeout.Changing the default timeout may be useful, for example, if the buffering proxy that necessitated enabling long-polling in the first place has a shorter timeout for hanging GET requests, in which case setting the long-polling timeout to a shorter value, such as 25 seconds, may fix prematurely-closed hanging GET requests. For example, see https://github.com/firebase/firebase-js-sdk/issues/6987. | ## ExperimentalLongPollingOptions.idleHttpRequestTimeoutSeconds -The desired maximum timeout interval (in seconds) to complete a long polling GET response. Valid values are integers between 5 and 30, inclusive. +The desired maximum timeout interval, in seconds, to complete a long-polling GET response. Valid values are integers between 5 and 30, inclusive. + +By default, when long-polling is used the "hanging GET" request sent by the client times out after 30 seconds. To request a different timeout from the server, set this setting with the desired timeout. -By default, when long polling is used the "hanging GET" request sent by the client times out after 30 seconds. To request a different timeout from the server, set this setting with the desired timeout. This may be useful, for example, if the buffering proxy that necessitated enabling long polling in the first place has a shorter timeout for hanging GET requests, in which case setting the long-polling timeout to a shorter value, such as 25 seconds, may fix prematurely-closed hanging GET requests. +Changing the default timeout may be useful, for example, if the buffering proxy that necessitated enabling long-polling in the first place has a shorter timeout for hanging GET requests, in which case setting the long-polling timeout to a shorter value, such as 25 seconds, may fix prematurely-closed hanging GET requests. For example, see https://github.com/firebase/firebase-js-sdk/issues/6987. Signature: diff --git a/docs-devsite/firestore_.firestoresettings.md b/docs-devsite/firestore_.firestoresettings.md index 85827dcd2de..d5beb7e03c7 100644 --- a/docs-devsite/firestore_.firestoresettings.md +++ b/docs-devsite/firestore_.firestoresettings.md @@ -25,7 +25,7 @@ export declare interface FirestoreSettings | [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. | | [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. | -| [experimentalLongPollingOptions](./firestore_.firestoresettings.md#firestoresettingsexperimentallongpollingoptions) | [ExperimentalLongPollingOptions](./firestore_.experimentallongpollingoptions.md#experimentallongpollingoptions_interface) | Options for long polling when it is used.These options are only used if experimentalForceLongPolling is true or if experimentalAutoDetectLongPolling is true and the auto-detection determined that long-polling was needed. Otherwise, they have no effect. | +| [experimentalLongPollingOptions](./firestore_.firestoresettings.md#firestoresettingsexperimentallongpollingoptions) | [ExperimentalLongPollingOptions](./firestore_.experimentallongpollingoptions.md#experimentallongpollingoptions_interface) | Options that configure the SDK’s underlying network transport (WebChannel) when long-polling is used.These options are only used if experimentalForceLongPolling is true or if experimentalAutoDetectLongPolling is true and the auto-detection determined that long-polling was needed. Otherwise, these options have no effect. | | [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. | | [localCache](./firestore_.firestoresettings.md#firestoresettingslocalcache) | [FirestoreLocalCache](./firestore_.md#firestorelocalcache) | Specifies the cache used by the SDK. Available options are MemoryLocalCache and IndexedDbLocalCache, each with different configuration options.When unspecified, MemoryLocalCache will be used by default.NOTE: setting this field and cacheSizeBytes at the same time will throw exception during SDK initialization. Instead, using the configuration in the FirestoreLocalCache object to specify the cache size. | @@ -71,9 +71,9 @@ experimentalForceLongPolling?: boolean; ## FirestoreSettings.experimentalLongPollingOptions -Options for long polling when it is used. +Options that configure the SDK’s underlying network transport (WebChannel) when long-polling is used. -These options are only used if `experimentalForceLongPolling` is true or if `experimentalAutoDetectLongPolling` is true and the auto-detection determined that long-polling was needed. Otherwise, they have no effect. +These options are only used if `experimentalForceLongPolling` is true or if `experimentalAutoDetectLongPolling` is true and the auto-detection determined that long-polling was needed. Otherwise, these options have no effect. Signature: diff --git a/docs-devsite/firestore_.md b/docs-devsite/firestore_.md index 51f7e294e8c..f098996a13d 100644 --- a/docs-devsite/firestore_.md +++ b/docs-devsite/firestore_.md @@ -147,7 +147,7 @@ https://github.com/firebase/firebase-js-sdk | [AggregateSpec](./firestore_.aggregatespec.md#aggregatespec_interface) | Specifies a set of aggregations and their aliases. | | [DocumentChange](./firestore_.documentchange.md#documentchange_interface) | A DocumentChange represents a change to the documents matching a query. It contains the document affected and the type of change that occurred. | | [DocumentData](./firestore_.documentdata.md#documentdata_interface) | Document data (for use with [setDoc()](./firestore_lite.md#setdoc)) consists of fields mapped to values. | -| [ExperimentalLongPollingOptions](./firestore_.experimentallongpollingoptions.md#experimentallongpollingoptions_interface) | Options for long polling when it is used.See FirestoreSettings.experimentalAutoDetectLongPolling, FirestoreSettings.experimentalForceLongPolling, and FirestoreSettings.experimentalLongPollingOptions. | +| [ExperimentalLongPollingOptions](./firestore_.experimentallongpollingoptions.md#experimentallongpollingoptions_interface) | Options that configure the SDK’s underlying network transport (WebChannel) when long-polling is used.Note: This interface is "experimental" and is subject to change.See FirestoreSettings.experimentalAutoDetectLongPolling, FirestoreSettings.experimentalForceLongPolling, and FirestoreSettings.experimentalLongPollingOptions. | | [FirestoreDataConverter](./firestore_.firestoredataconverter.md#firestoredataconverter_interface) | Converter used by withConverter() to transform user objects of type T into Firestore data.Using the converter allows you to specify generic type arguments when storing and retrieving objects from Firestore. | | [FirestoreSettings](./firestore_.firestoresettings.md#firestoresettings_interface) | Specifies custom configurations for your Cloud Firestore instance. You must set these before invoking any other methods. | | [Index](./firestore_.index.md#index_interface) | (BETA) The SDK definition of a Firestore index. | From 82ab9de7a21114a72972ea7b8f49756f7026f3be Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Tue, 25 Apr 2023 16:31:37 -0400 Subject: [PATCH 16/21] api council feedback - part 1 --- .../firestore/src/api/long_polling_options.ts | 11 +- packages/firestore/src/lite-api/settings.ts | 4 +- .../firestore/test/unit/api/database.test.ts | 100 +++++++++++------- 3 files changed, 70 insertions(+), 45 deletions(-) diff --git a/packages/firestore/src/api/long_polling_options.ts b/packages/firestore/src/api/long_polling_options.ts index 7ec1a915521..49eb81c3d4b 100644 --- a/packages/firestore/src/api/long_polling_options.ts +++ b/packages/firestore/src/api/long_polling_options.ts @@ -42,7 +42,7 @@ export interface ExperimentalLongPollingOptions { * prematurely-closed hanging GET requests. * For example, see https://github.com/firebase/firebase-js-sdk/issues/6987. */ - idleHttpRequestTimeoutSeconds?: number; + timeoutSeconds?: number; } /** @@ -52,10 +52,7 @@ export function longPollingOptionsEqual( options1: ExperimentalLongPollingOptions, options2: ExperimentalLongPollingOptions ): boolean { - return ( - options1.idleHttpRequestTimeoutSeconds === - options2.idleHttpRequestTimeoutSeconds - ); + return options1.timeoutSeconds === options2.timeoutSeconds; } /** @@ -67,8 +64,8 @@ export function cloneLongPollingOptions( ): ExperimentalLongPollingOptions { const clone: ExperimentalLongPollingOptions = {}; - if (options.idleHttpRequestTimeoutSeconds !== undefined) { - clone.idleHttpRequestTimeoutSeconds = options.idleHttpRequestTimeoutSeconds; + if (options.timeoutSeconds !== undefined) { + clone.timeoutSeconds = options.timeoutSeconds; } return clone; diff --git a/packages/firestore/src/lite-api/settings.ts b/packages/firestore/src/lite-api/settings.ts index 6db623d3609..b2e78061e43 100644 --- a/packages/firestore/src/lite-api/settings.ts +++ b/packages/firestore/src/lite-api/settings.ts @@ -198,11 +198,11 @@ function validateLongPollingOptions( options: ExperimentalLongPollingOptions ): void { if (options.idleHttpRequestTimeoutSeconds !== undefined) { - if (!Number.isInteger(options.idleHttpRequestTimeoutSeconds)) { + if (isNaN(options.idleHttpRequestTimeoutSeconds)) { throw new FirestoreError( Code.INVALID_ARGUMENT, `invalid long polling timeout: ` + - `${options.idleHttpRequestTimeoutSeconds} (must be an integer)` + `${options.idleHttpRequestTimeoutSeconds} (must not be NaN)` ); } if ( diff --git a/packages/firestore/test/unit/api/database.test.ts b/packages/firestore/test/unit/api/database.test.ts index 54698bce97b..5c6c1618ad1 100644 --- a/packages/firestore/test/unit/api/database.test.ts +++ b/packages/firestore/test/unit/api/database.test.ts @@ -265,7 +265,7 @@ describe('SnapshotMetadata', () => { }); }); -describe('Settings', () => { +describe.only('Settings', () => { it('can not use mutually exclusive settings together', () => { // Use a new instance of Firestore in order to configure settings. const db = newTestFirestore(); @@ -359,101 +359,129 @@ describe('Settings', () => { expect(db._getSettings().experimentalForceLongPolling).to.be.false; }); - it('idleHttpRequestTimeoutSeconds is undefined by default', () => { + it('timeoutSeconds is undefined by default', () => { // Use a new instance of Firestore in order to configure settings. const db = newTestFirestore(); - expect( - db._getSettings().experimentalLongPollingOptions - .idleHttpRequestTimeoutSeconds - ).to.be.undefined; + expect(db._getSettings().experimentalLongPollingOptions.timeoutSeconds).to + .be.undefined; }); - it('idleHttpRequestTimeoutSeconds minimum value is allowed', () => { + it('timeoutSeconds minimum value is allowed', () => { // Use a new instance of Firestore in order to configure settings. const db = newTestFirestore(); - db._setSettings({ - experimentalLongPollingOptions: { idleHttpRequestTimeoutSeconds: 5 } - }); + db._setSettings({ experimentalLongPollingOptions: { timeoutSeconds: 5 } }); expect( - db._getSettings().experimentalLongPollingOptions - .idleHttpRequestTimeoutSeconds + db._getSettings().experimentalLongPollingOptions.timeoutSeconds ).to.equal(5); }); - it('idleHttpRequestTimeoutSeconds maximum value is allowed', () => { + it('timeoutSeconds maximum value is allowed', () => { // Use a new instance of Firestore in order to configure settings. const db = newTestFirestore(); - db._setSettings({ - experimentalLongPollingOptions: { idleHttpRequestTimeoutSeconds: 30 } - }); + db._setSettings({ experimentalLongPollingOptions: { timeoutSeconds: 30 } }); expect( - db._getSettings().experimentalLongPollingOptions - .idleHttpRequestTimeoutSeconds + db._getSettings().experimentalLongPollingOptions.timeoutSeconds ).to.equal(30); }); - it('idleHttpRequestTimeoutSeconds typical value is allowed', () => { + it('timeoutSeconds typical value is allowed', () => { + // Use a new instance of Firestore in order to configure settings. + const db = newTestFirestore(); + db._setSettings({ experimentalLongPollingOptions: { timeoutSeconds: 25 } }); + expect( + db._getSettings().experimentalLongPollingOptions.timeoutSeconds + ).to.equal(25); + }); + + it('timeoutSeconds floating point value is allowed', () => { // Use a new instance of Firestore in order to configure settings. const db = newTestFirestore(); db._setSettings({ - experimentalLongPollingOptions: { idleHttpRequestTimeoutSeconds: 25 } + experimentalLongPollingOptions: { timeoutSeconds: 12.3456 } }); expect( - db._getSettings().experimentalLongPollingOptions - .idleHttpRequestTimeoutSeconds - ).to.equal(25); + db._getSettings().experimentalLongPollingOptions.timeoutSeconds + ).to.equal(12.3456); }); - it('idleHttpRequestTimeoutSeconds value one less than minimum throws', () => { + it('timeoutSeconds value one less than minimum throws', () => { // Use a new instance of Firestore in order to configure settings. const db = newTestFirestore(); expect(() => - db._setSettings({ - experimentalLongPollingOptions: { idleHttpRequestTimeoutSeconds: 4 } - }) + db._setSettings({ experimentalLongPollingOptions: { timeoutSeconds: 4 } }) ).to.throw(/invalid.*timeout.*4.*\(.*5.*\)/i); }); - it('idleHttpRequestTimeoutSeconds value one more than maximum throws', () => { + it('timeoutSeconds value one more than maximum throws', () => { // Use a new instance of Firestore in order to configure settings. const db = newTestFirestore(); expect(() => db._setSettings({ - experimentalLongPollingOptions: { idleHttpRequestTimeoutSeconds: 31 } + experimentalLongPollingOptions: { timeoutSeconds: 31 } }) ).to.throw(/invalid.*timeout.*31.*\(.*30.*\)/i); }); - it('idleHttpRequestTimeoutSeconds value of 0 throws', () => { + it('timeoutSeconds value of 0 throws', () => { + // Use a new instance of Firestore in order to configure settings. + const db = newTestFirestore(); + expect(() => + db._setSettings({ experimentalLongPollingOptions: { timeoutSeconds: 0 } }) + ).to.throw(/invalid.*timeout.*0.*\(.*5.*\)/i); + }); + + it('timeoutSeconds value of -0 throws', () => { // Use a new instance of Firestore in order to configure settings. const db = newTestFirestore(); expect(() => db._setSettings({ - experimentalLongPollingOptions: { idleHttpRequestTimeoutSeconds: 0 } + experimentalLongPollingOptions: { timeoutSeconds: -0 } }) ).to.throw(/invalid.*timeout.*0.*\(.*5.*\)/i); }); - it('idleHttpRequestTimeoutSeconds value of -1 throws', () => { + it('timeoutSeconds value of -1 throws', () => { // Use a new instance of Firestore in order to configure settings. const db = newTestFirestore(); expect(() => db._setSettings({ - experimentalLongPollingOptions: { idleHttpRequestTimeoutSeconds: -1 } + experimentalLongPollingOptions: { timeoutSeconds: -1 } }) ).to.throw(/invalid.*timeout.*-1.*\(.*5.*\)/i); }); - it('idleHttpRequestTimeoutSeconds non-integral value throws', () => { + it('timeoutSeconds value of -infinity throws', () => { + // Use a new instance of Firestore in order to configure settings. + const db = newTestFirestore(); + expect(() => + db._setSettings({ + experimentalLongPollingOptions: { + timeoutSeconds: Number.NEGATIVE_INFINITY + } + }) + ).to.throw(/invalid.*timeout.*-Infinity.*\(.*5.*\)/i); + }); + + it('timeoutSeconds value of +infinity throws', () => { // Use a new instance of Firestore in order to configure settings. const db = newTestFirestore(); expect(() => db._setSettings({ experimentalLongPollingOptions: { - idleHttpRequestTimeoutSeconds: 123.456 + timeoutSeconds: Number.POSITIVE_INFINITY } }) - ).to.throw(/invalid.*timeout.*123.456.*\(.*integer.*\)/i); + ).to.throw(/invalid.*timeout.*Infinity.*\(.*30.*\)/i); + }); + + it('timeoutSeconds value of NaN throws', () => { + // Use a new instance of Firestore in order to configure settings. + const db = newTestFirestore(); + expect(() => + db._setSettings({ + experimentalLongPollingOptions: { timeoutSeconds: Number.NaN } + }) + ).to.throw(/invalid.*timeout.*NaN/i); }); it('long polling autoDetect=[something truthy] should be coerced to true', () => { From d1d703fca741d252a2b7e635b2f3b834b9e3089a Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Tue, 25 Apr 2023 20:52:55 -0400 Subject: [PATCH 17/21] api council feedback - part 2 --- packages/firestore/src/lite-api/settings.ts | 20 ++++------- .../platform/browser/webchannel_connection.ts | 7 ++-- .../firestore/test/unit/api/database.test.ts | 2 +- .../unit/api/long_polling_options.test.ts | 36 ++++++++----------- 4 files changed, 25 insertions(+), 40 deletions(-) diff --git a/packages/firestore/src/lite-api/settings.ts b/packages/firestore/src/lite-api/settings.ts index b2e78061e43..7dfea42db92 100644 --- a/packages/firestore/src/lite-api/settings.ts +++ b/packages/firestore/src/lite-api/settings.ts @@ -197,31 +197,25 @@ export class FirestoreSettingsImpl { function validateLongPollingOptions( options: ExperimentalLongPollingOptions ): void { - if (options.idleHttpRequestTimeoutSeconds !== undefined) { - if (isNaN(options.idleHttpRequestTimeoutSeconds)) { + if (options.timeoutSeconds !== undefined) { + if (isNaN(options.timeoutSeconds)) { throw new FirestoreError( Code.INVALID_ARGUMENT, `invalid long polling timeout: ` + - `${options.idleHttpRequestTimeoutSeconds} (must not be NaN)` + `${options.timeoutSeconds} (must not be NaN)` ); } - if ( - options.idleHttpRequestTimeoutSeconds < MIN_LONG_POLLING_TIMEOUT_SECONDS - ) { + if (options.timeoutSeconds < MIN_LONG_POLLING_TIMEOUT_SECONDS) { throw new FirestoreError( Code.INVALID_ARGUMENT, - `invalid long polling timeout: ` + - `${options.idleHttpRequestTimeoutSeconds} ` + + `invalid long polling timeout: ${options.timeoutSeconds} ` + `(minimum allowed value is ${MIN_LONG_POLLING_TIMEOUT_SECONDS})` ); } - if ( - options.idleHttpRequestTimeoutSeconds > MAX_LONG_POLLING_TIMEOUT_SECONDS - ) { + if (options.timeoutSeconds > MAX_LONG_POLLING_TIMEOUT_SECONDS) { throw new FirestoreError( Code.INVALID_ARGUMENT, - `invalid long polling timeout: ` + - `${options.idleHttpRequestTimeoutSeconds} ` + + `invalid long polling timeout: ${options.timeoutSeconds} ` + `(maximum allowed value is ${MAX_LONG_POLLING_TIMEOUT_SECONDS})` ); } diff --git a/packages/firestore/src/platform/browser/webchannel_connection.ts b/packages/firestore/src/platform/browser/webchannel_connection.ts index 49869be96f9..5082418cf69 100644 --- a/packages/firestore/src/platform/browser/webchannel_connection.ts +++ b/packages/firestore/src/platform/browser/webchannel_connection.ts @@ -203,10 +203,9 @@ export class WebChannelConnection extends RestConnection { detectBufferingProxy: this.autoDetectLongPolling }; - const idleHttpRequestTimeoutSeconds = - this.longPollingOptions.idleHttpRequestTimeoutSeconds; - if (idleHttpRequestTimeoutSeconds !== undefined) { - request.longPollingTimeout = idleHttpRequestTimeoutSeconds * 1000; + const longPollingTimeoutSeconds = this.longPollingOptions.timeoutSeconds; + if (longPollingTimeoutSeconds !== undefined) { + request.longPollingTimeout = Math.round(longPollingTimeoutSeconds * 1000); } if (this.useFetchStreams) { diff --git a/packages/firestore/test/unit/api/database.test.ts b/packages/firestore/test/unit/api/database.test.ts index 5c6c1618ad1..94b67257d2a 100644 --- a/packages/firestore/test/unit/api/database.test.ts +++ b/packages/firestore/test/unit/api/database.test.ts @@ -265,7 +265,7 @@ describe('SnapshotMetadata', () => { }); }); -describe.only('Settings', () => { +describe('Settings', () => { it('can not use mutually exclusive settings together', () => { // Use a new instance of Firestore in order to configure settings. const db = newTestFirestore(); diff --git a/packages/firestore/test/unit/api/long_polling_options.test.ts b/packages/firestore/test/unit/api/long_polling_options.test.ts index ee429293e4d..228fecb9bf9 100644 --- a/packages/firestore/test/unit/api/long_polling_options.test.ts +++ b/packages/firestore/test/unit/api/long_polling_options.test.ts @@ -28,33 +28,25 @@ describe('long_polling_options', () => { expect(longPollingOptionsEqual({}, {})).to.be.true; }); - it('longPollingOptionsEqual() should return true if both objects have the same idleHttpRequestTimeoutSeconds', () => { - const options1: ExperimentalLongPollingOptions = { - idleHttpRequestTimeoutSeconds: 123 - }; - const options2: ExperimentalLongPollingOptions = { - idleHttpRequestTimeoutSeconds: 123 - }; + it('longPollingOptionsEqual() should return true if both objects have the same timeoutSeconds', () => { + const options1: ExperimentalLongPollingOptions = { timeoutSeconds: 123 }; + const options2: ExperimentalLongPollingOptions = { timeoutSeconds: 123 }; expect(longPollingOptionsEqual(options1, options2)).to.be.true; }); - it('longPollingOptionsEqual() should return false if the objects have different idleHttpRequestTimeoutSeconds', () => { - const options1: ExperimentalLongPollingOptions = { - idleHttpRequestTimeoutSeconds: 123 - }; - const options2: ExperimentalLongPollingOptions = { - idleHttpRequestTimeoutSeconds: 321 - }; + it('longPollingOptionsEqual() should return false if the objects have different timeoutSeconds', () => { + const options1: ExperimentalLongPollingOptions = { timeoutSeconds: 123 }; + const options2: ExperimentalLongPollingOptions = { timeoutSeconds: 321 }; expect(longPollingOptionsEqual(options1, options2)).to.be.false; }); it('longPollingOptionsEqual() should ignore properties not defined in ExperimentalLongPollingOptions', () => { const options1 = { - idleHttpRequestTimeoutSeconds: 123, + timeoutSeconds: 123, someOtherProperty: 42 } as ExperimentalLongPollingOptions; const options2 = { - idleHttpRequestTimeoutSeconds: 123, + timeoutSeconds: 123, someOtherProperty: 24 } as ExperimentalLongPollingOptions; expect(longPollingOptionsEqual(options1, options2)).to.be.true; @@ -64,19 +56,19 @@ describe('long_polling_options', () => { expect(cloneLongPollingOptions({})).to.deep.equal({}); }); - it('cloneLongPollingOptions() should copy idleHttpRequestTimeoutSeconds', () => { - expect( - cloneLongPollingOptions({ idleHttpRequestTimeoutSeconds: 1234 }) - ).to.deep.equal({ idleHttpRequestTimeoutSeconds: 1234 }); + it('cloneLongPollingOptions() should copy timeoutSeconds', () => { + expect(cloneLongPollingOptions({ timeoutSeconds: 1234 })).to.deep.equal({ + timeoutSeconds: 1234 + }); }); it('cloneLongPollingOptions() should not copy properties not defined in ExperimentalLongPollingOptions', () => { const options = { - idleHttpRequestTimeoutSeconds: 1234, + timeoutSeconds: 1234, someOtherProperty: 42 } as ExperimentalLongPollingOptions; expect(cloneLongPollingOptions(options)).to.deep.equal({ - idleHttpRequestTimeoutSeconds: 1234 + timeoutSeconds: 1234 }); }); }); From be90ef006dad8d59017c365a830e9d6751d83108 Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Tue, 25 Apr 2023 20:54:17 -0400 Subject: [PATCH 18/21] api council feedback - part 3 --- packages/firestore/src/api/long_polling_options.ts | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/packages/firestore/src/api/long_polling_options.ts b/packages/firestore/src/api/long_polling_options.ts index 49eb81c3d4b..8da3fd30c7b 100644 --- a/packages/firestore/src/api/long_polling_options.ts +++ b/packages/firestore/src/api/long_polling_options.ts @@ -28,8 +28,9 @@ export interface ExperimentalLongPollingOptions { /** * The desired maximum timeout interval, in seconds, to complete a - * long-polling GET response. Valid values are integers between 5 and 30, - * inclusive. + * long-polling GET response. Valid values are between 5 and 30, inclusive. + * Floating point values are allowed and will be rounded to the nearest + * millisecond. * * By default, when long-polling is used the "hanging GET" request sent by * the client times out after 30 seconds. To request a different timeout From 4431c8ec380346e81e008329a50be80edb8bc67e Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Tue, 25 Apr 2023 21:00:01 -0400 Subject: [PATCH 19/21] yarn docgen devsite --- common/api-review/firestore.api.md | 2 +- docs-devsite/firestore_.experimentallongpollingoptions.md | 8 ++++---- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/common/api-review/firestore.api.md b/common/api-review/firestore.api.md index 0a35d7ec7b8..580320b0aa3 100644 --- a/common/api-review/firestore.api.md +++ b/common/api-review/firestore.api.md @@ -186,7 +186,7 @@ export function endBefore(...fieldValues: unknown[]): QueryEndAtConstraint; // @public export interface ExperimentalLongPollingOptions { - idleHttpRequestTimeoutSeconds?: number; + timeoutSeconds?: number; } // @public diff --git a/docs-devsite/firestore_.experimentallongpollingoptions.md b/docs-devsite/firestore_.experimentallongpollingoptions.md index 6ec7a8c90c1..2a9ef50af46 100644 --- a/docs-devsite/firestore_.experimentallongpollingoptions.md +++ b/docs-devsite/firestore_.experimentallongpollingoptions.md @@ -26,11 +26,11 @@ export declare interface ExperimentalLongPollingOptions | Property | Type | Description | | --- | --- | --- | -| [idleHttpRequestTimeoutSeconds](./firestore_.experimentallongpollingoptions.md#experimentallongpollingoptionsidlehttprequesttimeoutseconds) | number | The desired maximum timeout interval, in seconds, to complete a long-polling GET response. Valid values are integers between 5 and 30, inclusive.By default, when long-polling is used the "hanging GET" request sent by the client times out after 30 seconds. To request a different timeout from the server, set this setting with the desired timeout.Changing the default timeout may be useful, for example, if the buffering proxy that necessitated enabling long-polling in the first place has a shorter timeout for hanging GET requests, in which case setting the long-polling timeout to a shorter value, such as 25 seconds, may fix prematurely-closed hanging GET requests. For example, see https://github.com/firebase/firebase-js-sdk/issues/6987. | +| [timeoutSeconds](./firestore_.experimentallongpollingoptions.md#experimentallongpollingoptionstimeoutseconds) | number | The desired maximum timeout interval, in seconds, to complete a long-polling GET response. Valid values are between 5 and 30, inclusive. Floating point values are allowed and will be rounded to the nearest millisecond.By default, when long-polling is used the "hanging GET" request sent by the client times out after 30 seconds. To request a different timeout from the server, set this setting with the desired timeout.Changing the default timeout may be useful, for example, if the buffering proxy that necessitated enabling long-polling in the first place has a shorter timeout for hanging GET requests, in which case setting the long-polling timeout to a shorter value, such as 25 seconds, may fix prematurely-closed hanging GET requests. For example, see https://github.com/firebase/firebase-js-sdk/issues/6987. | -## ExperimentalLongPollingOptions.idleHttpRequestTimeoutSeconds +## ExperimentalLongPollingOptions.timeoutSeconds -The desired maximum timeout interval, in seconds, to complete a long-polling GET response. Valid values are integers between 5 and 30, inclusive. +The desired maximum timeout interval, in seconds, to complete a long-polling GET response. Valid values are between 5 and 30, inclusive. Floating point values are allowed and will be rounded to the nearest millisecond. By default, when long-polling is used the "hanging GET" request sent by the client times out after 30 seconds. To request a different timeout from the server, set this setting with the desired timeout. @@ -39,5 +39,5 @@ Changing the default timeout may be useful, for example, if the buffering proxy Signature: ```typescript -idleHttpRequestTimeoutSeconds?: number; +timeoutSeconds?: number; ``` From dc64cab310ae698a4d7fadaf802d41ea6e26b1d2 Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Thu, 27 Apr 2023 15:27:29 -0400 Subject: [PATCH 20/21] Replace internal link with b/266868871 --- packages/firestore/src/lite-api/settings.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/firestore/src/lite-api/settings.ts b/packages/firestore/src/lite-api/settings.ts index 7dfea42db92..3a731689e83 100644 --- a/packages/firestore/src/lite-api/settings.ts +++ b/packages/firestore/src/lite-api/settings.ts @@ -37,7 +37,7 @@ export const DEFAULT_SSL = true; // The minimum long-polling timeout is hardcoded on the server. The value here // should be kept in sync with the value used by the server, as the server will // silently ignore a value below the minimum and fall back to the default. -// Googlers see http://google3/net/webchannel/internal/webchannel_config.cc;l=118;rcl=510899643 +// Googlers see b/266868871 for relevant discussion. const MIN_LONG_POLLING_TIMEOUT_SECONDS = 5; // No maximum long-polling timeout is configured in the server, and defaults to From 3bd125dbd325121a0db17d7866dd4ebe9e98aad5 Mon Sep 17 00:00:00 2001 From: Denver Coneybeare Date: Fri, 28 Apr 2023 15:58:22 -0400 Subject: [PATCH 21/21] yarn changeset --- .changeset/witty-wasps-play.md | 6 ++++++ 1 file changed, 6 insertions(+) create mode 100644 .changeset/witty-wasps-play.md diff --git a/.changeset/witty-wasps-play.md b/.changeset/witty-wasps-play.md new file mode 100644 index 00000000000..2b28862ce2e --- /dev/null +++ b/.changeset/witty-wasps-play.md @@ -0,0 +1,6 @@ +--- +'@firebase/firestore': minor +'firebase': minor +--- + +Added the ability to configure the long-polling hanging get request timeout using the new `idleHttpRequestTimeoutSeconds` setting