Follow up to reconnection callback fix / Fix reconnect methods not firing on laptop awakening

[marcaaron]

This PR should help organize the reconnection logic and event listeners that were previously hanging out in API. I also removed the logic that would turn off the AppState listener on web and desktop since I initially figured it would be too aggressive. But with reports that things are not showing in the LHN, maybe it's not a bad idea to be aggressive about reconnect callbacks firing.

Update:

Also, added a mechanism to help detect when a laptop has been closed. This is not something that we get with NetInfo or the Network Information API it depends on.

Curious to get some thoughts @cead22 @AndrewGable @tgolen

Fixed Issues

Fixes https://github.com/Expensify/Expensify/issues/142726
Fixes https://github.com/Expensify/Expensify/issues/141817

Tests

Screenshots

❌

[marcaaron]

Some more context on this change here -> https://github.com/Expensify/Expensify/issues/141817#issuecomment-703862314

[tgolen]

Sounds like a good thing to try!

[marcaaron]

Heads up, I earmarked a fix for https://github.com/Expensify/Expensify/issues/141817 into this PR

If anyone objects to that I'm happy to separate it into a new PR.

[tgolen]

Sounds like a good thing to try!

[cead22]

Curious for everyone's thoughts on my comment

[cead22]

Is it worth using setTimeout instead of setInterval (and maybe establishing this as a best practice for JS)?

This is an old "best practice" but I think it's still valid, and the idea behind it is that if there's an uncaught error that prevents clearInterval from running, this function will be running in a "loop" forever.

To avoid that, I think we can use setTimeout instead like so:

We define it as a recursive function that calls setTimeout

function listenForReconnect() {
...

    // define it
    var checkConnectivity = () => {
        setTimeout(() => {
            const currentTime = (new Date()).getTime();
            if (currentTime > (lastTime + 5000)) {
                triggerReconnectionCallbacks();
            }
            lastTime = currentTime;
            checkConnectivity();
        }, 2000)
    };

    // call it once
    checkConnectivity()
    ...
}

And when we stop listening, we re-define it as a no-op

function stopListeningForReconnect() {
    // make it a no-op
    checkConnectivity = () => {};
    ...
}

[marcaaron]

That's a really interesting point. And I guess my only question would be what exactly would cause the clearInterval() to never run in this case? We are calling it via redirectToSignIn(), but I'm not seeing how (or which) exception would prevent it from running. And if there was such an exception then wouldn't that same exception prevent you from setting checkConnectivity as a no-op?

[cead22]

what exactly would cause the clearInterval() to never run in this case?

I don't know, it's mostly a defensive measure in case we ever write a bug that causes an error that isn't caught

And if there was such an exception then wouldn't that same exception prevent you from setting checkConnectivity as a no-op?

Duh, yeah 🤦. I was looking for the right way to do this since I haven't thought about it in a while, so clearly this isn't it, but I bet there is a way. I'll look into it, and will merge this now

[cead22]

Oh actually, I think what happens is that if there's an uncaught exception and we don't make checkConnectivity a no-op, it doesn't really matter because we didn't call setInterval, and thus, the execution of checkConnectivity will be interrupted too. Does that make sense, and do you agree with that being how it'd work?

[marcaaron]

Ok, I think I get it now! You are saying that if something inside the checkConnectivity method fails then we will stop looping. Therefore it's better to use recursion here since we don't want to keep calling a method that we already know is going to fail.

I think that makes sense, but another perspective might be that setInterval is only bad if we allow these unhandled errors to exist unhandled. Consider this case:

myInterval = setInterval(() => {
	try {
        breakStuff();
    } catch(err) {
        clearInterval(myInterval);
        throw err;
    }
}, 1000);

I like this better because we don't need to no-op anything or use recursion. If we encounter an error then we just decide to stop the loop and throw the error.

[tgolen]

I like that idea of using a try/catch. I think that's cleaner than setTimeout. I'd also like to see that added as a utility like safeSetInterval or something (though not necessary for this).

[cead22]

You are saying that if something inside the checkConnectivity method fails then we will stop looping.

No, not really. I think any uncaught error can stop the execution of JS code, is that not the case with React?

[marcaaron]

React is JavaScript and follows the same rules. The suggestion that an uncaught error from anywhere could somehow stop a recursive setTimeout() I think is wrong? Timers have their own execution context and can only be "broken" if an error is thrown inside the timer.

[cead22]

Yup, not from anywhere, but if triggerReconnectionCallbacks(); or anything that that function calls throws, then the interval won't be cleared. So the try/catch you propose should work well