feat(firestore, web): expose `webExperimentalForceLongPolling`, `webExperimentalAutoDetectLongPolling` and `timeoutSeconds` on web by SelaseKay · Pull Request #13201 · firebase/flutterfire ·

Description

Expose experimentalForceLongPolling on firestore Settings

Related Issues

Checklist

Before you create this PR confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]).
This will ensure a smooth and quick review process. Updating the pubspec.yaml and changelogs is not required.

I read the Contributor Guide and followed the process outlined there for submitting PRs.
My PR includes unit or integration tests for all changed/updated/fixed behaviors (See Contributor Guide).
All existing and new tests are passing.
I updated/added relevant documentation (doc comments with ///).
The analyzer (melos run analyze) does not report any problems on my PR.
I read and followed the Flutter Style Guide.
I signed the CLA.
I am willing to follow-up on review comments in a timely manner.

Breaking Change

Does your PR require plugin users to manually update their apps to accommodate your change?

Yes, this is a breaking change.
No, this is not a breaking change.

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

Looks nice, I think you'll also need to add

The PR can be marked as feat(firestore, web)

Lyokone · 2024-08-20T14:41:28Z

packages/cloud_firestore/cloud_firestore/lib/src/firestore.dart

@@ -315,6 +315,7 @@ class FirebaseFirestore extends FirebasePluginPlatform {
 persistenceEnabled: settings.persistenceEnabled,
 host: settings.host,
 cacheSizeBytes: settings.cacheSizeBytes,
+experimentalForceLongPolling: settings.experimentalForceLongPolling,


Since it's only available for web, I would rename it webExperimentalForceLongPolling

…lForceLongPolling since it applies to web only.

…ebExperimentalForceLongPolling, timeoutSeconds)

Lyokone · 2024-08-21T08:32:23Z

packages/cloud_firestore/cloud_firestore/lib/src/firestore.dart

@@ -315,6 +315,8 @@ class FirebaseFirestore extends FirebasePluginPlatform {
 persistenceEnabled: settings.persistenceEnabled,
 host: settings.host,
 cacheSizeBytes: settings.cacheSizeBytes,
+webExperimentalForceLongPolling: settings.webExperimentalForceLongPolling,
+webExperimentalAutoDetectLongPolling: settings.webExperimentalAutoDetectLongPolling,


I'm not seeing timeoutSeconds exposed in the user facing lib. I think it should be hidden being an objet like in the original web SDK. experimentalLongPollingOptions

Lyokone · 2024-08-21T08:32:53Z

packages/cloud_firestore/cloud_firestore_platform_interface/lib/src/settings.dart

+this.webExperimentalForceLongPolling = true,
+this.webExperimentalAutoDetectLongPolling = true,
+this.timeoutSeconds = 30,


We should probably not set any default so we can rely on the default value of the underlying JS SDK.

Lyokone · 2024-08-21T08:34:00Z

packages/cloud_firestore/cloud_firestore_web/lib/cloud_firestore_web.dart

@@ -152,18 +152,30 @@ class FirebaseFirestoreWeb extends FirebaseFirestorePlatform {
 ));
 }

+dynamic experimentalLongPollingOptions = firestore_interop.ExperimentalLongPollingOptions(


We try to avoid using dynamic, you can probably type that as JSAny.

Lyokone · 2024-08-21T09:58:40Z

packages/cloud_firestore/cloud_firestore_platform_interface/lib/src/settings.dart

+/// 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.


This one can be shorten to match Options that configure the SDK’s underlying network transport (WebChannel) when long-polling is used. on https://firebase.google.com/docs/reference/js/firestore_.experimentallongpollingoptions.md#experimentallongpollingoptions_interface

Lyokone · 2024-08-21T09:58:43Z

packages/cloud_firestore/cloud_firestore_platform_interface/lib/src/settings.dart

+///This is very similar to [webExperimentalForceLongPolling], but only uses long-polling if required.
+final bool? webExperimentalAutoDetectLongPolling;
+
+final WebExperimentalLongPollingOptions? webExperimentalLongPollingOptions;


Missing a documentation here
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.

Lyokone · 2024-08-21T09:58:49Z

packages/cloud_firestore/cloud_firestore_platform_interface/lib/src/settings.dart

+///This is very similar to [webExperimentalForceLongPolling], but only uses long-polling if required.
+final bool? webExperimentalAutoDetectLongPolling;
+
+final WebExperimentalLongPollingOptions? webExperimentalLongPollingOptions;


Missing a documentation here
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.

@Lyokone

Probably should add to firestore e2e tests to see what happens when it is passed via interop layer. Other than that, and @Lyokone's requests, LGTM

jude.kwashie added 2 commits August 20, 2024 14:20

fix(firestore): expose on FireStoreSettings

721b46d

fix(firestore): expose on FireStoreSettings

93a56f8

SelaseKay closed this Aug 20, 2024

Lyokone reviewed Aug 20, 2024
View reviewed changes

fix(firestore): change experimentalForceLongPolling to webExperimenta…

921b46b
…lForceLongPolling since it applies to web only.

SelaseKay reopened this Aug 20, 2024

Lyokone changed the title ~~fix(firestore): expose experimentalForceLongPolling on FireStoreSettings~~ feat(firestore, web): expose experimentalForceLongPolling on web Aug 20, 2024

feat(firestore, web): expose (webExperimentalAutoDetectLongPolling, w…

bfc142b
…ebExperimentalForceLongPolling, timeoutSeconds)

SelaseKay changed the title ~~feat(firestore, web): expose experimentalForceLongPolling on web~~ feat(firestore, web): expose webExperimentalForceLongPolling, webExperimentalAutoDetectLongPolling and timeoutSeconds on web Aug 21, 2024

correct variable name

51f2056

Lyokone requested changes Aug 21, 2024
View reviewed changes

SelaseKay added 2 commits August 21, 2024 09:47

abstract timeoutDuration within an object

4b6f081

remove description on webExperimentalLongPollingOptions field

4cce27a

Lyokone reviewed Aug 21, 2024
View reviewed changes

SelaseKay added 2 commits August 21, 2024 10:43

resolve documentation

4c08161

fix analyzer issues

b8f2e58

russellwheatley requested changes Aug 23, 2024
View reviewed changes

SelaseKay added 6 commits August 23, 2024 18:02

test(firestore, web): add e2e tests for long polling fields

b8a9b92

test(firestore, web): add e2e tests for long polling fields

8283e58

remove skip field from test

ac53e87

remove unused import

75ec398

tests(firestore,web): make adjustments long polling tests

9ba6925

tests(firestore,web): make adjustments long polling tests

2c36f41

Lyokone approved these changes Sep 3, 2024
View reviewed changes

russellwheatley approved these changes Sep 3, 2024
View reviewed changes

Lyokone merged commit 6ec2a10 into firebase:main Sep 3, 2024
19 of 21 checks passed

firebase locked and limited conversation to collaborators Oct 4, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Lyokone Aug 20, 2024

Uh oh!

SelaseKay Aug 20, 2024

Uh oh!

Lyokone Aug 21, 2024

Uh oh!

Lyokone Aug 21, 2024

Uh oh!

Lyokone Aug 21, 2024

Uh oh!

Lyokone Aug 21, 2024

Uh oh!

Lyokone Aug 21, 2024

Uh oh!

Lyokone Aug 21, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Conversation

Description

Related Issues

Checklist

Breaking Change

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!