Skip to content

Firestore: Enable auto-detection of long-polling networking mode #7236

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 25 commits into from
May 8, 2023
Merged
Show file tree
Hide file tree
Changes from 23 commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
f062376
set experimentalAutoDetectLongPolling default value to true
dconeybe Apr 20, 2023
65d5e9c
changeset
dconeybe Apr 20, 2023
959c0e7
Firestore: database.test.ts: add unit tests for long polling settings…
dconeybe Apr 20, 2023
7f52fe4
Merge branch 'LongPollingSettingsTests' into AutoDetectLongPollingEna…
dconeybe Apr 20, 2023
ac752d1
Update documentation of experimentalAutoDetectLongPolling to document…
dconeybe Apr 20, 2023
8218910
reword changeset
dconeybe Apr 20, 2023
09d6f31
yarn docgen devsite
dconeybe Apr 20, 2023
40f2018
Merge remote-tracking branch 'origin/master' into AutoDetectLongPolli…
dconeybe Apr 20, 2023
eb854dc
Firestore: settings.ts: very minor refactor of long-polling logic.
dconeybe Apr 20, 2023
75b2c9b
Merge branch 'LongPollingLogicTweak' into AutoDetectLongPollingEnable…
dconeybe Apr 20, 2023
ef8665f
invert negative 'if' statement and ensure that experimentalAutoDetect…
dconeybe Apr 20, 2023
e0e0741
Merge branch 'LongPollingLogicTweak' into AutoDetectLongPollingEnable…
dconeybe Apr 20, 2023
8e84d43
add DEFAULT_AUTO_DETECT_LONG_POLLING and coerce to boolean
dconeybe Apr 20, 2023
3e2be99
Firestore: database.test.ts: add tests for coercing experimentalForce…
dconeybe Apr 20, 2023
d8fccbe
yarn prettier
dconeybe Apr 20, 2023
7427858
tweak unit test descriptions
dconeybe Apr 20, 2023
81a61b4
add `// eslint-disable-next-line @typescript-eslint/no-explicit-any` …
dconeybe Apr 20, 2023
10aba36
Merge branch 'LongPollingCoerceToBooleanTests' into LongPollingLogicT…
dconeybe Apr 20, 2023
17519c3
invert a negative 'if' statement
dconeybe Apr 20, 2023
7522b43
Merge remote-tracking branch 'remotes/origin/dconeybe/LongPollingLogi…
dconeybe Apr 20, 2023
c27a49a
Merge remote-tracking branch 'origin/master' into LongPollingLogicTweak
dconeybe Apr 20, 2023
a9696c5
Merge branch 'LongPollingLogicTweak' into AutoDetectLongPollingEnable…
dconeybe Apr 20, 2023
b64dc66
Merge remote-tracking branch 'origin/master' into AutoDetectLongPolli…
dconeybe Apr 21, 2023
f166d1d
tweak api docs for experimentalAutoDetectLongPolling, as suggested in…
dconeybe Apr 21, 2023
e58f70b
Merge remote-tracking branch 'origin/master' into AutoDetectLongPolli…
dconeybe May 5, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions .changeset/long-lemons-change.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
---
'@firebase/firestore': minor
'firebase': minor
---

Enabled long-polling networking mode auto detection by default. It can be explicitly disabled by setting `FirestoreSettings.experimentalForceLongPolling` to `false`.
4 changes: 2 additions & 2 deletions docs-devsite/firestore_.firestoresettings.md
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ export declare interface FirestoreSettings
| Property | Type | Description |
| --- | --- | --- |
| [cacheSizeBytes](./firestore_.firestoresettings.md#firestoresettingscachesizebytes) | number | NOTE: This field will be deprecated in a future major release. Use <code>cache</code> 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 <code>CACHE_SIZE_UNLIMITED</code> 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 <code>experimentalForceLongPolling</code>, but only uses long-polling if required.<!-- -->This setting will likely be enabled by default in future releases and cannot be combined with <code>experimentalForceLongPolling</code>. |
| [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 <code>experimentalForceLongPolling</code>, but only uses long-polling if required.<!-- -->After having had a default value of <code>false</code> since its inception 4 years ago, the default value of this setting was changed in the most recent release to <code>true</code>. That is, auto-detection of long polling is now enabled by default. To disable it, set this setting to <code>false</code>, and please open a GitHub issue to share your problems with long-polling auto-detection. |
| [experimentalForceLongPolling](./firestore_.firestoresettings.md#firestoresettingsexperimentalforcelongpolling) | boolean | Forces the SDK’s underlying network transport (WebChannel) to use long-polling. Each response from the backend will be closed immediately after the backend sends data (by default responses are kept open in case the backend has more data to send). This avoids incompatibility issues with certain proxies, antivirus software, etc. that incorrectly buffer traffic indefinitely. Use of this option will cause some performance degradation though.<!-- -->This setting cannot be used with <code>experimentalAutoDetectLongPolling</code> and may be removed in a future release. If you find yourself using it to work around a specific network reliability issue, please tell us about it in https://github.com/firebase/firebase-js-sdk/issues/1674. |
| [host](./firestore_.firestoresettings.md#firestoresettingshost) | string | The hostname to connect to. |
| [ignoreUndefinedProperties](./firestore_.firestoresettings.md#firestoresettingsignoreundefinedproperties) | boolean | Whether to skip nested properties that are set to <code>undefined</code> during object serialization. If set to <code>true</code>, these properties are skipped and not written to Firestore. If set to <code>false</code> or omitted, the SDK throws an exception when it encounters properties of type <code>undefined</code>. |
Expand All @@ -48,7 +48,7 @@ cacheSizeBytes?: number;

Configures the SDK's underlying transport (WebChannel) to automatically detect if long-polling should be used. This is very similar to `experimentalForceLongPolling`<!-- -->, but only uses long-polling if required.

This setting will likely be enabled by default in future releases and cannot be combined with `experimentalForceLongPolling`<!-- -->.
After having had a default value of `false` since its inception 4 years ago, the default value of this setting was changed in the most recent release to `true`<!-- -->. That is, auto-detection of long polling is now enabled by default. To disable it, set this setting to `false`<!-- -->, and please open a GitHub issue to share your problems with long-polling auto-detection.

<b>Signature:</b>

Expand Down
7 changes: 5 additions & 2 deletions packages/firestore/src/api/settings.ts
Original file line number Diff line number Diff line change
Expand Up @@ -88,8 +88,11 @@ export interface FirestoreSettings extends LiteSettings {
* detect if long-polling should be used. This is very similar to
* `experimentalForceLongPolling`, but only uses long-polling if required.
*
* This setting will likely be enabled by default in future releases and
* cannot be combined with `experimentalForceLongPolling`.
* After having had a default value of `false` since its inception 4 years
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note to self: update this documentation immediately after release to mention the year "2019" instead of "4 years ago" and the actual version number instead of "the most recent release".

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Googlers see b/278957890 for details.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: Just say "since 2019" and say "and changed in mid 2023" so you don't need to worry about it later?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. I still think I want to go back and change it to the specific version and release date so that customers can more easily correlate their issues with the release. But at least with your wording suggestion it's not as time-critical to get that change in.

* ago, the default value of this setting was changed in the most recent
* release to `true`. That is, auto-detection of long polling is now enabled
* by default. To disable it, set this setting to `false`, and please open a
* GitHub issue to share your problems with long-polling auto-detection.
*/
experimentalAutoDetectLongPolling?: boolean;
}
2 changes: 1 addition & 1 deletion packages/firestore/src/lite-api/settings.ts
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ import { validateIsNotUsedTogether } from '../util/input_validation';
export const DEFAULT_HOST = 'firestore.googleapis.com';
export const DEFAULT_SSL = true;

const DEFAULT_AUTO_DETECT_LONG_POLLING = false;
const DEFAULT_AUTO_DETECT_LONG_POLLING = true;

/**
* Specifies custom configurations for your Cloud Firestore instance.
Expand Down
8 changes: 4 additions & 4 deletions packages/firestore/test/unit/api/database.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -279,10 +279,10 @@ describe('Settings', () => {
);
});

it('long polling should be disabled by default', () => {
it('long polling should be in auto-detect mode by default', () => {
// Use a new instance of Firestore in order to configure settings.
const db = newTestFirestore();
expect(db._getSettings().experimentalAutoDetectLongPolling).to.be.false;
expect(db._getSettings().experimentalAutoDetectLongPolling).to.be.true;
expect(db._getSettings().experimentalForceLongPolling).to.be.false;
});

Expand All @@ -306,13 +306,13 @@ describe('Settings', () => {
expect(db._getSettings().experimentalForceLongPolling).to.be.false;
});

it('long polling should be disabled if force=false', () => {
it('long polling should be in auto-detect mode if force=false', () => {
// Use a new instance of Firestore in order to configure settings.
const db = newTestFirestore();
db._setSettings({
experimentalForceLongPolling: false
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

by the looks of it, whether experimentalForceLongPolling is set to false or not does not change the settings below (as it looks the same from the test above).

However, setting experimentalForceLongPolling to false should actually disable auto detect long polling too, is that right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If both experimentalForceLongPolling=true and experimentalAutoDetectLongPolling=true then an exception will be thrown:

it('can not use mutually exclusive settings together', () => {
// Use a new instance of Firestore in order to configure settings.
const db = newTestFirestore();
expect(() =>
db._setSettings({
experimentalForceLongPolling: true,
experimentalAutoDetectLongPolling: true
})
).to.throw(
`experimentalForceLongPolling and experimentalAutoDetectLongPolling cannot be used together.`
);
});

Those two settings are mutually exclusive. As a result, only one of forceLongPolling and detectBufferingProxy will be true in WebChannelOptions

forceLongPolling: this.forceLongPolling,
detectBufferingProxy: this.autoDetectLongPolling

Does that address your concern?

});
expect(db._getSettings().experimentalAutoDetectLongPolling).to.be.false;
expect(db._getSettings().experimentalAutoDetectLongPolling).to.be.true;
expect(db._getSettings().experimentalForceLongPolling).to.be.false;
});

Expand Down