Firestore: Enable auto-detection of long-polling networking mode #7236
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
🦋 Changeset detectedLatest commit: e58f70b The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
… that its default value is now true
dconeybe
commented
Apr 20, 2023
| @@ -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 | |||
There was a problem hiding this comment.
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".
There was a problem hiding this comment.
Googlers see b/278957890 for details.
There was a problem hiding this comment.
Suggestion: Just say "since 2019" and say "and changed in mid 2023" so you don't need to worry about it later?
There was a problem hiding this comment.
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.
Size Report 1Affected Products
Test Logs |
Size Analysis Report 1Affected Products
Test Logs |
…LongPolling is boolean
…LongPolling and experimentalAutoDetectLongPolling to boolean
…lines to make lint happy
…cTweak' into AutoDetectLongPollingEnabledByDefault
…ngEnabledByDefault
rafikhan
approved these changes
Apr 21, 2023
| @@ -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 | |||
There was a problem hiding this comment.
Suggestion: Just say "since 2019" and say "and changed in mid 2023" so you don't need to worry about it later?
|
@egilmorez Could you review for docs? |
markarndt
approved these changes
Apr 28, 2023
…ngEnabledByDefault
sampajano
reviewed
May 8, 2023
| @@ -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 | |||
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
If both experimentalForceLongPolling=true and experimentalAutoDetectLongPolling=true then an exception will be thrown:
firebase-js-sdk/packages/firestore/test/unit/api/database.test.ts
Lines 269 to 280 in e147219
Those two settings are mutually exclusive. As a result, only one of forceLongPolling and detectBufferingProxy will be true in WebChannelOptions
Does that address your concern?
Merged
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Firestore has a setting named
experimentalAutoDetectLongPollingwhich can be set totrueto enable some heuristics to detect if enabling "long-polling" networking mode fixes communication problems with the Firestore backend servers. See #1674 for details.For the past 4 years this "auto-detect" mode was opt-in; however, we now have confidence that it is safe to enable for everyone. Therefore, this PR enables the "auto-detect" mode by default. If you are experiencing networking issues you can try disabling the long-polling auto-detection by setting
FirestoreSettings.experimentalAutoDetectLongPollingtofalse. If you do this, please open an issue in this GitHub repository to report your problems and mention "long-polling" in the issue title.