Skip to content
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

Fix experimentalAutoDetectLongPolling and add logging #4078

Merged
merged 5 commits into from
Nov 19, 2020

Conversation

schmidt-sebastian
Copy link
Contributor

@schmidt-sebastian schmidt-sebastian commented Nov 17, 2020

This fixes a bug in experimentalAutoDetectLongPolling, as the WebChannel flag was not added to the closure externs and as such mangled. It also adds logging for each mode.

Verified with a test app in Chrome (albeit only for the "no buffering proxy" scenario).

@changeset-bot
Copy link

changeset-bot bot commented Nov 17, 2020

🦋 Changeset detected

Latest commit: 17824e9

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 9 packages
Name Type
@firebase/firestore Patch
@firebase/webchannel-wrapper Patch
firebase Patch
@firebase/rules-unit-testing Patch

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

@schmidt-sebastian schmidt-sebastian changed the title WIP: Add RequestStats support for Firestore's WebChannel connection Fix experimentalAutoDetectForceLongPolling and add logging Nov 17, 2020
@schmidt-sebastian schmidt-sebastian changed the title Fix experimentalAutoDetectForceLongPolling and add logging Fix experimentalAutoDetectLongPolling and add logging Nov 17, 2020
@firebase firebase deleted a comment from google-oss-bot Nov 17, 2020
Copy link
Contributor

@rafikhan rafikhan left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -348,6 +356,16 @@ export class WebChannelConnection extends RestConnection {
}
);

unguardedEventListen<StatEvent>(requestStats, Event.STAT_EVENT, event => {
if (!event) return;
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this line necessary? If so, under what conditions would it happen. Might be worth adding a comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seems like unguardedEventListen can be modified safely to assume that there is always an event. Fixed.

@schmidt-sebastian schmidt-sebastian marked this pull request as ready for review November 17, 2020 22:58
@google-oss-bot
Copy link
Contributor

google-oss-bot commented Nov 17, 2020

Size Analysis Report

Affected Products

No changes between base commit (0c4f794) and head commit (9437671).

Test Logs

@schmidt-sebastian schmidt-sebastian merged commit 9822e12 into master Nov 19, 2020
@google-oss-bot google-oss-bot mentioned this pull request Nov 20, 2020
@firebase firebase locked and limited conversation to collaborators Dec 20, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants