-
Notifications
You must be signed in to change notification settings - Fork 939
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
Changes from 23 commits
f062376
65d5e9c
959c0e7
7f52fe4
ac752d1
8218910
09d6f31
40f2018
eb854dc
75b2c9b
ef8665f
e0e0741
8e84d43
3e2be99
d8fccbe
7427858
81a61b4
10aba36
17519c3
7522b43
c27a49a
a9696c5
b64dc66
f166d1d
e58f70b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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`. |
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -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; | ||||||||||||||||||||||||||||||
}); | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
|
@@ -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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. by the looks of it, whether However, setting There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If both 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 firebase-js-sdk/packages/firestore/src/platform/browser/webchannel_connection.ts Lines 202 to 203 in e147219
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; | ||||||||||||||||||||||||||||||
}); | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
|
There was a problem hiding this comment.
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".
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.