-
Notifications
You must be signed in to change notification settings - Fork 41
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
feat: offline state recovery for Filter subscription #2049
base: master
Are you sure you want to change the base?
Conversation
ba8867c
to
4c446d5
Compare
size-limit report 📦
|
} | ||
} | ||
private isConnectedToNetwork: boolean = false; | ||
private isConnectedToWakuNetwork: boolean = 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.
imo it's not immediately clear what being connected to network means vs being connected to waku network. might be worth adding a comment
private startBrowserNetworkStatusListener(): void { | ||
try { | ||
window.addEventListener("online", this.onEventHandlers["online"]); | ||
window.addEventListener("offline", this.onEventHandlers["offline"]); |
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.
I'm concerned about relying solely on window events to detect when there is an internet connection. Is it possible for the event listener to miss an online event? This could cause the app to be stuck in offline state when there is actually a connection present.
It might be worth manually checking the boolean value of navigator.onLine
as a fallback when trying to take an action that depends on isConnectedToNetwork
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.
I agree. When we rely on these event listeners, we inherently rely on Libp2p's ability to be able to perform during these environmental changes that I'm not sure if reliable enough.
Relying on the client itself, ie, navigator
sounds like a great way to do this, pending tests
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.
Can we introduce some tests that validate the diff for this? We can do this by mocking disconnection
} | ||
this.contentTopics = contentTopics; | ||
this.subscribeOptions = options; | ||
this.startBackgroundProcess(options); |
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.
let's be verbose in what this backgroundProcess
is, ie, keep alive pings
this.startNetworkMonitoring(); | ||
} | ||
|
||
private stopBackgroundRoutine(): void { |
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.
Discrepancy in name: backgroundProcess
and backgroundRoutine
Problem
In case light node goes offline some steps should be taken to recover as per RFC recommendation
Solution
Implement recovery by doing
ping
andre-subscription
if needed.Notes