Add ConnectivityMonitor #1808
Merged
Add ConnectivityMonitor #1808
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
thebrianchen
requested review from
Feiyang1,
hiranya911 and
hsubox76
as code owners
thebrianchen
requested review from
wilhuff
and removed request for
hiranya911,
Feiyang1 and
hsubox76
wilhuff
reviewed
May 21, 2019
packages/firestore/src/platform_browser/browser_connectivity_monitor.ts
Outdated
Show resolved
Hide resolved
packages/firestore/src/platform_browser/browser_connectivity_monitor.ts
Outdated
Show resolved
Hide resolved
wilhuff
approved these changes
May 21, 2019
| * that each platform will have its own system-dependent implementation. | ||
| */ | ||
| export interface ConnectivityMonitor { | ||
| addCallback(callback: ConnectivityMonitorCallback): void; |
There was a problem hiding this comment.
More comments on what these do please.
Specifically, does the callback get called on the initial state?
mikelehen
reviewed
May 21, 2019
There was a problem hiding this comment.
Just as a note, we ended up disabling online/offline monitoring for RTDB on Cordova (
) due to bugginess. That was years ago, so hopefully the problem is resolved... but I just wanted to mention it so it's on your radar just in case we get complaints about connection issues on Cordova after this releases.
| } | ||
|
|
||
| private configureNetworkMonitoring(): void { | ||
| window.addEventListener('online', this.networkAvailableListener); |
There was a problem hiding this comment.
@thebrianchen I believe we need to fix this still as per our earlier discussion.
Ideally, you would change BrowserWindow to only return BrowserConnectivityMonitor if window is defined. The smaller fix would be to add a typeof window !== undefined check here. As merged, I believe this change will cause issues on Electron and other browser-like environments.
There was a problem hiding this comment.
This is probably causing issue #1824.
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.
No description provided.