LongPoll #814
LongPoll #814
Conversation
mahboubii
commented
Nov 22, 2021
•
mahboubii
commented
- Retry
- Recover state
- Test
|
Size Change: +14.3 kB (5%) 🔍 Total Size: 277 kB
|
| @@ -256,14 +266,14 @@ export class StableWSConnection< | |||
| * @return {ConnectAPIResponse<ChannelType, CommandType, UserType>} Promise that completes once the first health check message is received | |||
| */ | |||
| async _connect() { | |||
| if (this.isConnecting) return; // simply ignore _connect if it's currently trying to connect | |||
| if (this.isConnecting || this.isDisconnected) return; // simply ignore _connect if it's currently trying to connect | |||
There was a problem hiding this comment.
This will affect background/foreground logic on mobile, no? Once disconnected, user won't be able to connect again when app comes back to foreground
There was a problem hiding this comment.
needs maintenance of it in callback to me
There was a problem hiding this comment.
what do you call to connect again when the app comes to foreground?
There was a problem hiding this comment.
if you call disconnect(), and then call connectUser() again, it should work since connect will set isDisconnected to false
There was a problem hiding this comment.
So on RN we use openConnection/closeConnection
There was a problem hiding this comment.
then it should be OK, but still worth manual testing with both longpoll enabled/disabled situation
| * connect try to open a longpoll request | ||
| * @param reconnect should be false for first call and true for subsequent calls to keep the connection alive and call recoverState | ||
| */ | ||
| connect = async (reconnect = false) => { |
There was a problem hiding this comment.
this connect has reconnect as argument, while the other has timeout. Maybe better to use an object on both.
| connect = async (reconnect = false) => { | |
| connect = async (options = { reconnect: false }) => { |
| } catch (err) { | ||
| // run fallback only if it's WS/Network error and not a normal API error | ||
| // make sure browser is online before even trying the longpoll | ||
| if (this.options.enableWSFallback && isWSFailure(err) && isOnline()) { |
There was a problem hiding this comment.
I wonder if we shouldn't remove thhe online check. If you're offline the this.wsConnection.connect fails and isWsFailure will return false anyways. On the other hand, if it could be the case that you have a wsFailure and you're offline, in RN this check wouldn't work, and we would trigger the fallback while offline. I'm not sure if I'm missing something, though. Does it make sense?
There was a problem hiding this comment.
we did a bad job with naming here but isWSFailure here returns true if you are offline, basically, isWSFailure is in contrast with isAPIFailure. isWSFailure is true when we couldn't reach API for any reason, including network issue
There was a problem hiding this comment.
Following up on this: so I just tested on RN. If you access window.navigator it looks like following:
{"product": "ReactNative"}
And thus isOnline() function will return always undefined -> fallback will never be executed. Maybe we can provide a way so that RN SDK can let JS client know about the online status (e.g., we can provide js client with async function to fetch online status).
