Improve internet-connectivity state a bit; upgrade @react-native-community/netinfo. #4940

chrisbobbe · 2021-08-03T21:29:14Z

Now, we're specifically using a bit of state that says whether we can expect to reach the Internet. And we show a notice if it's taken unexpectedly long to determine the state (16 seconds).

Getting this state right is good in general, for user experience. But it'd also become more critical if we adapt our request retry-and-timeout logic to act on the Internet reachability state, which would be reasonable to do.

gnprice

Thanks @chrisbobbe ! These changes will be good. Comments below, mainly on the implementation of useHasStayedTrueForMs.

gnprice · 2021-08-11T23:02:10Z

flow-typed/@react-native-community/netinfo_v5.x.x.js

+    | NetInfoWimaxState
+    | NetInfoVpnState
+    | NetInfoOtherState;
+  declare export type NetInfoState = NetInfoDisconnectedStates | NetInfoConnectedStates;


Neat! This type is pretty detailed.

gnprice · 2021-08-11T23:10:05Z

src/boot/AppEventHandlers.js

+        //
+        // (Several of those parameters are configurable -- timeout durations,
+        // URL, etc.)
+        netInfoState.isInternetReachable,


online state: Switch to `isInternetReachable`, from `connectionType`. This is hopefully more accurate about what we actually want to know: "can the app expect to reach the Internet?" Not just "is there some network connection?".

Cool.

One thing happening here is that our old logic was equivalent to using the isConnected property (except that with @react-native-community/netinfo v6 that property started being null sometimes instead of false.) So using that would have been a bit cleaner for us to do in the first place, plus with the v6 upgrade it gives us the distinction between null and false.

Then switching from (effectively) isConnected to isInternetReachable makes it false or possibly null in some cases where it previously would have been true. That's the result of asking the more on-point question.

gnprice · 2021-08-11T23:28:58Z

src/common/OfflineNotice.js

+  useEffect(() => {
+    if (value) {
+      timeoutId.current = setTimeout(() => {
+        setResult(true);
+      }, duration);
+    } else {
+      clearTimeout(timeoutId.current);
+      timeoutId.current = null;
+      setResult(false);
+    }
+  }, [value, duration]);


Hmm. Won't this clobber the old timeout? That is, if we have:

Render with value true.

Render again with value true, before that timeout completes.

then we'll forget about the old timeout and set a new one.

I guess the original timeout will still end up firing… but because timeoutId.current won't know about it anymore, it'll end up firing even if the value then changes to false.

Oh, and I guess this is a latent bug as it stands, because it only triggers if the effect gets rerun. Which only happens if value or duration changes, and currently we never change duration. Still, good to fix the latent bug in case we reuse this -- plus I suspect the code will be somewhat easier to reason through in a fixed version.

Separately: we should clear this timeout if the component unmounts, so we don't have a leaked timeout trying to belatedly call setResult.

useEffect supports this -- have its callback return the cleanup function:
https://reactjs.org/docs/hooks-reference.html#cleaning-up-an-effect

Oh, and I guess this is a latent bug as it stands, because it only triggers if the effect gets rerun.

Ah, yeah, I think I was sort of counting on this fact for the implementation. I think I should use usePrevious(value) in there somewhere for explicitness (and so it doesn't require callers to never change duration); I'll try that.

chrisbobbe · 2021-08-12T22:12:05Z

Thanks for the review! Revision pushed.

gnprice

Thanks! Partial thoughts below.

gnprice · 2021-08-20T00:29:38Z

src/common/OfflineNotice.js

+  useEffect(() => {
+    const startTimer = () => {
+      timeoutId.current = setTimeout(() => {
+        setResult(true);
+      }, duration);
+    };
+
+    const resetTimer = () => {
+      clearTimeout(timeoutId.current);
+      timeoutId.current = null;
+      setResult(false);
+    };
+
+    if (prevValue !== true && value) {
+      startTimer();
+    } else if (prevValue === true && !value) {
+      resetTimer();
+    }
+
+    return resetTimer;
+  }, [prevValue, value, duration]);


Hmm. What happens if duration changes (while value doesn't)?

I think the consequence of that will be:

This effect gets invoked again.

… But, before any of the logic inside the effect gets to run, the last effect's teardown callback is invoked. That's resetTimer. So the existing timeout gets cleared.

So I think this still ends up clobbering the old timeout.

This logic does seem tricky to pin down. I'll stare at this a bit and see if I can come up with a clean way to do it.

I guess it's not too bad that we clear the old timeout if duration does change -- certainly that's no longer quite the timeout we want anyway, because duration is what determines the desired timing. Though this still isn't really ideal behavior in that case: for example if duration keeps changing back and forth, we'll never get around to realizing that the value has been true for well over duration.

But also, because we're depending on prevValue as well as value here, if we simply get a rerender (with no duration change):

First render, with value true (and prevValue null.)

Second render, with value still true, but now prevValue also true.

then the effect will get rerun, and we'll cancel the timeout.

I guess that can't happen more than once in a row, but still not the desired behavior.

gnprice · 2021-08-24T01:28:55Z

I've just pushed a revision that takes a bit of a different approach to the issue of duration changing in useHasStayedTrueForMs: we declare that the caller isn't supposed to do that, plus assert that they don't. (With the assertion skipped in production mode, to avoid adding a useRef to track the previous value.) Please take a look!

Also added a couple of tweaks atop the changes to usePrevious. All the changes to usePrevious end up being moot with this revision, but I think the improvements to it are useful anyway and would be good to have for the future. A demo of how my added commits would be helpful is in 59c2c59, in a demo branch: we get to have the initial value be false, and the resulting type is boolean rather than boolean | null, simplifying the conditionals on it.

chrisbobbe · 2021-08-26T23:58:38Z

Thanks, those changes look good! Should I squash as I see appropriate, or would you prefer to do that @gnprice?

…lers. The version we're on now (5.9.10) is much later than 3.2.1, which it looks like was what informed the types written down in AppEventHandlers. One nice-looking thing we get is `isInternetReachable`, which should actually tell us whether the Internet is reachable (not just whether there's an active network connection).

And update the libdef by transferring over the changes made in the two TypeScript files it's based on. This handles the only breaking change announced: > - useNetinfo: When the connection state is unknown, the > `isConnected` and `isInternetReachable` properties are now set > to `null` rather than `false`. This allow you to easily detect > the initial "unknown" state before the state is detected and set > to a boolean.

This is hopefully more accurate about what we actually want to know: "can the app expect to reach the Internet?" Not just "is there some network connection?". Getting this right is already important for a nice user experience. But it'll be more critical if we adapt our request retry-and-timeout logic to be aware of the Internet reachability state, which is reasonable to want to do. In 6.0.0, `isInternetReachable` started being `boolean | null`; see the comment about when it's `null`. Since `null` is a meaningful third state, adjust the Redux plumbing to allow `null` for `state.session.isOnline` so we can do something distinct in the UI.

As Greg pointed out a few weeks ago [1], `usePrevious` has had a "funny quirk where on the first call, it takes the "previous" value to be the current one." Instead, let the caller decide what the value should be on the first render, or just have it be `null` if the caller is fine with that. [1] zulip#4914 (comment)

Mostly to get the summary line back down to a line, pushing a detail out into the prose body. Also update the reference to the upstream source we based this on, now that we've improved it in a way that changes its interface.

The main motivation for this is that it makes the types behave more uniformly between the case where `initValue` is or isn't provided: this way, the value we actually use for it is always the value passed, rather than sometimes being that and sometimes of a different type. That in turn will let us tighten the types so that when we have a real initial value to provide, the return type becomes simply `T` rather than `T | null` or `T | void`.

This way, in cases where we have an initial value that's actually of the main value's type `T` (like false or zero), we can see that the return value is always of type `T`, rather than have the type-checker think that it might be null or undefined.

And log what we know from NetInfo to Sentry.

gnprice · 2021-08-27T01:15:16Z

Cool -- going ahead and merging.

Should I squash as I see appropriate,

Yeah, so if I'd written the whole branch myself, then I'd squash at least the first three of these:
4957259 usePrevious [nfc]: Default to null on first render.
be0dda5 usePrevious [nfc]: Tweak jsdoc after recent change.
e033a06 usePrevious [nfc]: Default to undefined instead of null.
646cdde usePrevious [nfc]: Let types support a non-nullish initValue.

and maybe all four. If you'd written the whole branch, then I expect you'd do something like that too. They basically make one change (the first commit, which you wrote), and then revise it (the subsequent commits, which I added.)

But there's nothing broken about the first commit, so it's not essential that the changes need to be squashed in. I regularly merge branches that have a sequence like this, when there's something that I tweak at the same time as I merge. In those cases the separate commits are helpful for showing what I changed and why -- basically the same reason that I left them unsquashed when pushing this revision the other day.

This commit sequence seems clear enough, so in the interest of not spending more time than necessary in logistics, I'm happy just merging 🙂

(If one of the commits was broken in some way -- either in our tests, or from a user perspective -- then I'd definitely want to squash in any fixes. That's the one real hard rule of commit sequences for us, that we always aim to not have a step that's broken.)

This is our first use of `react-test-renderer`. It piggy-backs on our incorporation of Jest's "modern" fake-timer implementation in PRs zulip#4754 and zulip#4931. That was handy! I haven't yet found any test cases that fail with our implementation. (And I'd been hoping to, to debug an unexpected error!) But I did try pasting in an earlier iteration of the hook's implementation, from zulip#4940, that Greg had found bugs in by reading the code. Many of these tests failed on that buggy implementation, which is a good sign. Might as well keep these new tests, then, if they're not an unreasonable maintenance burden.

chrisbobbe added the dependencies Pull requests that update a dependency file label Aug 3, 2021

chrisbobbe requested review from gnprice and WesleyAC August 3, 2021 21:29

chrisbobbe force-pushed the pr-netinfo-upgrade branch from e9a7c9f to 370ef21 Compare August 6, 2021 19:32

gnprice reviewed Aug 11, 2021

View reviewed changes

chrisbobbe force-pushed the pr-netinfo-upgrade branch from 370ef21 to 28856f2 Compare August 12, 2021 22:12

gnprice reviewed Aug 20, 2021

View reviewed changes

gnprice force-pushed the pr-netinfo-upgrade branch from 28856f2 to f81449f Compare August 24, 2021 01:21

chrisbobbe and others added 10 commits August 26, 2021 17:48

usePrevious [nfc]: Tweak jsdoc after recent change.

be0dda5

Mostly to get the summary line back down to a line, pushing a detail out into the prose body. Also update the reference to the upstream source we based this on, now that we've improved it in a way that changes its interface.

reactUtils: Add useHasStayedTrueForMs.

da59c2b

reactUtils: Add useDebugAssertConstant; apply to that duration.

5a5b0de

OfflineNotice: Show an alert if we stay in an unknown state too long.

88b0a71

And log what we know from NetInfo to Sentry.

gnprice force-pushed the pr-netinfo-upgrade branch from f81449f to 88b0a71 Compare August 27, 2021 01:15

gnprice merged commit 88b0a71 into zulip:master Aug 27, 2021

chrisbobbe mentioned this pull request Sep 9, 2021

reactUtils tests: Start testing useHasStayedTrueForMs. #4997

Merged

chrisbobbe deleted the pr-netinfo-upgrade branch November 4, 2021 21:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve internet-connectivity state a bit; upgrade @react-native-community/netinfo. #4940

Improve internet-connectivity state a bit; upgrade @react-native-community/netinfo. #4940

chrisbobbe commented Aug 3, 2021 •

edited

Loading

gnprice left a comment

gnprice Aug 11, 2021

gnprice Aug 11, 2021

gnprice Aug 11, 2021

gnprice Aug 11, 2021

chrisbobbe Aug 12, 2021 •

edited

Loading

chrisbobbe commented Aug 12, 2021

gnprice left a comment

gnprice Aug 20, 2021

gnprice Aug 20, 2021

gnprice commented Aug 24, 2021

chrisbobbe commented Aug 26, 2021

gnprice commented Aug 27, 2021 •

edited

Loading

Improve internet-connectivity state a bit; upgrade @react-native-community/netinfo. #4940

Improve internet-connectivity state a bit; upgrade @react-native-community/netinfo. #4940

Conversation

chrisbobbe commented Aug 3, 2021 • edited Loading

gnprice left a comment

Choose a reason for hiding this comment

gnprice Aug 11, 2021

Choose a reason for hiding this comment

gnprice Aug 11, 2021

Choose a reason for hiding this comment

gnprice Aug 11, 2021

Choose a reason for hiding this comment

gnprice Aug 11, 2021

Choose a reason for hiding this comment

chrisbobbe Aug 12, 2021 • edited Loading

Choose a reason for hiding this comment

chrisbobbe commented Aug 12, 2021

gnprice left a comment

Choose a reason for hiding this comment

gnprice Aug 20, 2021

Choose a reason for hiding this comment

gnprice Aug 20, 2021

Choose a reason for hiding this comment

gnprice commented Aug 24, 2021

chrisbobbe commented Aug 26, 2021

gnprice commented Aug 27, 2021 • edited Loading

chrisbobbe commented Aug 3, 2021 •

edited

Loading

chrisbobbe Aug 12, 2021 •

edited

Loading

gnprice commented Aug 27, 2021 •

edited

Loading