events: Remove brittle `needsInitialFetch` system #5300

chrisbobbe · 2022-03-15T23:49:00Z

This is the next PR in my series toward better offline handling for the /register request.

We've long thought that the current logic is complex and fragile;
see for example
https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/Stuck.20on.20loading.20screen/near/907688

gnprice

Thanks @chrisbobbe! This code around needsInitialFetch has been a tangly part of our control flow -- very glad to see it cleaned up. Comments below.

gnprice · 2022-03-17T22:02:09Z

src/account/logoutActions.js

@@ -0,0 +1,18 @@
+/* @flow strict-local */


nit in commit message:

Since that register-queue code can end up calling `logout` [1], and

This footnote doesn't seem to exist.

gnprice · 2022-03-17T22:07:33Z

src/boot/AppDataFetcher.js

+  React.useEffect(() => {
+    init();
+  }, [init, needsInitialFetch, dispatch]);


nit: this dependency list can have just init, right?

gnprice · 2022-03-17T22:07:48Z

src/boot/StoreHydratedGate.js

+    () => {
+      init();
+    },


nit: this can just say init

init returns a Promise.

useEdgeTriggeredEffect, as well as useEffect and our new useConditionalEffect we discussed in the office on Thursday, won't take a callback that returns a Promise.

In fact, if I just pass an async function expression directly to useEffect—

useEffect(async () => 'foo');

—I get a complaint from react-hooks/exhaustive-deps, with some explanation:

Effect callbacks are synchronous to prevent race conditions. Put the async function inside: useEffect(() => { async function fetchData() { // You can await here const response = await MyAPI.getData(someId); // ... } fetchData(); }, [someId]); // Or [] if effect doesn't need props or state Learn more about data fetching with Hooks: https://reactjs.org/link/hooks-data-fetchingeslint[react-hooks/exhaustive-deps](https://github.com/facebook/react/issues/14920)

I see, yeah. Here we're happy for it to be an async thing we don't wait for, but it is good to have something making that explicit. Which this does, a little bit.

gnprice · 2022-03-17T22:13:24Z

src/events/eventActions.js

+ * Also do some miscellaneous other work we want to do when starting
+ * up, or regaining a network connection. We fetch private messages


I think this sentence can be deleted when we separate out the outbox-send and init-notifications bits -- that's what it's really about.

gnprice · 2022-03-17T22:15:45Z

src/events/eventActions.js

+ * (`fetchOlder` and `fetchNewer`), and to grab search results
+ * (`SearchMessagesScreen`).
+ */
+export const registerAndStartPolling = (): ThunkAction<Promise<void>> => async (


Let's say somewhere in the jsdoc for this that this is also called "the initial fetch". That phrase still appears in various comments and docs (try git grep -i initial.?fetch), as well as in lots of discussion history, so it's good to have some hint for people trying to look up what it's referring to.

Ah, good thought. Here's an important place where it shows up: https://zulip.readthedocs.io/en/latest/subsystems/events-system.html#the-initial-data-fetch

gnprice · 2022-03-17T22:32:43Z

src/events/eventActions.js

+  await dispatch(registerAndStartPolling());
+
+  // TODO(server-2.1): Drop this.
+  dispatch(fetchPrivateMessages());


This fetchPrivateMessages is covered already by the registerAndStartPolling, right?

Oh also in the commit message:

This time for DEAD_QUEUE: just directly dispatch `doInitialFetch` in

s/doInitialFetch/registerAndStartPolling and friends/, or words to that effect. Same in the previous commit.

gnprice · 2022-03-17T22:46:24Z

src/boot/AppDataFetcher.js

+  useEdgeTriggeredEffect(
+    () => {
+      init();
+    },
+    isHydrated,
+
+    // True because this component will have begun its life with
+    // `isHydrated: true`, and we still want to init. (We're in a descendant
+    // of HideIfNotHydrated.)
+    //
+    // TODO: Can move this logic upward, to where `isHydrated` can actually
+    //   be seen to change from `false` to `true`. HideIfNotHydrated sounds
+    //   like a natural place, and it'd mean one fewer component in the
+    //   tree.
+    true,
+  );


Given that isHydrated is always true, I believe this can be simplified to:

useEffect(init, []);

(Plus a comment explaining that the point is this runs upon hydration.)

That is, it will run on first render of this component, and never again.

I guess one thing about that line is that it would violate one of the Rules of Hooks -- it doesn't have a dependency on init, and in particular on (indirectly) hasAuth. And hasAuth really is non-constant.

But I think what that's really telling us is that useEdgeTriggeredEffect should only be used with a callback that's constant. (As it effectively is in the one existing use site, in ChatScreen.) And that the lint rules for the Rules of Hooks would ideally be supplemented by one that checks for that (or, to which we could somehow express that it should check for that.)

Then I think an explicit eslint-ignore line would be a better way to express breaking that rule than would getting around what the lint rules can see by using this other hook 😉

gnprice · 2022-03-17T22:55:13Z

docs/architecture/realtime.md

+* Which is invoked by `StoreHydratedGate` on startup, just if there's an
+  active, logged-in account.
+* It's also invoked in other crucial places:


Suggested change

* Which is invoked by `StoreHydratedGate` on startup, just if there's an

active, logged-in account.

* It's also invoked in other crucial places:

* In turn `registerAndStartPolling` is invoked in several situations:

* On startup, by `StoreHydratedGate`, just if there's an active, logged-in account.

(To group the different call sites together at the same level, and also to clarify the referent of "Which is invoked …".)

gnprice · 2022-03-17T23:02:24Z

src/boot/StoreHydratedGate.js

+    }
+  }, [dispatch, hasAuth]);
+
+  useEdgeTriggeredEffect(


In the final version, where this is in StoreHydratedGate and so isHydrated may start out false, I agree useEdgeTriggeredEffect is a good fit.

Continuing on from the comment above, though: I think the dependency on hasAuth should be taken as a genuine breach of the Rules of Hooks -- one that we're deliberately choosing here -- even though the ESLint plugin doesn't spot it. So there should be a comment explaining why that's OK. I think the point is basically that if hasAuth becomes true later, that's going to be either via ACCOUNT_SWITCH or LOGIN_SUCCESS, and each of those is accompanied by its own invocation of basically the same things as are in this init function.

gnprice · 2022-03-17T23:35:08Z

src/events/eventActions.js

-  // TODO: Remove this. No need to do this on DEAD_QUEUE or ACCOUNT_SWITCH:
-  //   https://chat.zulip.org/#narrow/stream/48-mobile/topic/.60remoteNotificationsRegistered.60.20listener.20too.20late/near/1148915
-  dispatch(initNotifications());


Remind me: was there something that actually goes wrong as a result of this, that removing it fixes? Or was it just a matter of feeling that this wasn't the right place for it?

As I think more about this action, I think it's actually in a situation very much parallel to sendOutbox: there's some data we have (be it our notifications token, or the user's messages they've tried to send); we want to get it to the server; when we eventually succeed at that, we'll update our data to reflect that, so we always have a to-do list of what we still want to send; and we want to keep trying in order to get it through to the server, until we succeed (or in the case of a message, perhaps time out.)

Those, and: we don't currently have a real principled way of driving those retries. Instead, we take the occasion of the initial fetch as a hook to get us to retry. It'd be good to get a principled way of doing them (for outbox, that's #3881), but until then every place we retry is potentially a helpful one, even if ad hoc.

So if there isn't a concrete problem that's solved by removing these initNotification calls, I think I'd prefer to actually keep them. After #3881, we can take whatever we work out for that and make something similar for driving these retries too.

Remind me: was there something that actually goes wrong as a result of this, that removing it fixes? Or was it just a matter of feeling that this wasn't the right place for it?

Yeah, as far as I remember it's the latter. I'll adjust.

Yeah, we'll have to think carefully about retries.

Even setting that aside, I've often found our code for registering push tokens pretty hard to follow, and wondered if we could make it better.

We quick-fixed a P0 bug #4573 with 6ffc7e1, and I don't think we've settled on a plan for something more permanent.

I think this is the description of what the relevant subsystem is aimed at:

For all servers we want notifications from, we should keep the server up-to-date with our current device token.

If we're logged into a server, we want notifications from it.

This suggests having some per-account code that runs with the account's LOGIN_SUCCESS.

Our device token could change between sessions of running the app, or during a session.

This suggests having some global code that runs on startup, and on learning (e.g., from an event) that the token has changed during this session. Global because the token is for the device, and also because we may need to tell multiple servers about the token.

I think there's a main reason why our code has developed such that it's not super clear how we answer these requirements: we're not supposed to ask iOS for the token until we're ready to pop up a permissions request, and we're supposed to do that only in a context where the permission would be useful to have. We've (reasonably) decided that the app enters that context when a /register completes, so that's when we request the token on iOS.

Here's an example of how the code is hard to follow. It occurred to me after your comment prompted me to keep thinking about this:

Our device token could change between sessions of running the app […].

This suggests having some global code that runs on startup […]. Global because the token is for the device, and also because we may need to tell multiple servers about the token.

Our "on-startup" code for this is initNotifications. That sure doesn't look like "some global code", does it? It's a ThunkAction, not a GlobalThunkAction. But then, if you sit and think about it and read some distant code, it's possible to convince yourself that all interested servers will indeed be notified if the device token changed between sessions of running the app, on iOS and Android. Better if the "on-startup" code were instead a GlobalThunkAction, with a nice jsdoc that mentioned that device tokens can change between sessions.

I have a draft from a year ago for a unified getAndReturnNotificationToken function that grabs the token in the platform-appropriate way. That specific draft may or may not end up being helpful, but I think if we chose one of the following, we could unify our separate iOS/Android code and thus remove lots of complexity:

iOS and Android both get the token immediately on startup (sad for iOS because the permission pop-up isn't deferred till after /register), or

iOS and Android both get the token after /register, where iOS gets it currently (maybe sad for Android because Android doesn't have this requirement that iOS has—but also it wouldn't be harmful, would it?)

I think this is the description of what the relevant subsystem is aimed at:

Yeah, that all seems right.

Here's an example of how the code is hard to follow. It occurred to me after your comment prompted me to keep thinking about this: […]

Our "on-startup" code for this is initNotifications. That sure doesn't look like "some global code", does it? It's a ThunkAction, not a GlobalThunkAction. But then, if you sit and think about it and read some distant code, it's possible to convince yourself that all interested servers will indeed be notified if the device token changed between sessions of running the app, on iOS and Android.

Hmm, indeed. I agree, that seems like a key point for why this logic is hard to follow.

Want to make an issue for this? That'll probably provide a better way to track it than this review subthread.

Sure, sounds good: #5329.

To replace useEdgeTriggeredEffect, which implicitly required a constant callback; see zulip#5300 (comment) and this bit of useEdgeTriggeredEffect's jsdoc: * The callback is not permitted to return a cleanup function, because it's * not clear what the semantics should be of when such a cleanup function * would be run.

Following Greg's explanation at zulip#5300 (comment)

chrisbobbe · 2022-03-22T19:39:06Z

Thanks for the review! Revision pushed.

To replace useEdgeTriggeredEffect, which implicitly required a constant callback; see zulip#5300 (comment) and this bit of useEdgeTriggeredEffect's jsdoc: > The callback is not permitted to return a cleanup function, > because it's not clear what the semantics should be of when such a > cleanup function would be run.

gnprice · 2022-04-05T04:39:32Z

src/boot/AppDataFetcher.js

+    },
+    // We just want to init once, just after the store rehydrates. This
+    // component will have begun its life at that time, since it's under
+    // HideIfNotHydrated.


nit:

Suggested change

// HideIfNotHydrated.

// StoreHydratedGate.

gnprice · 2022-04-05T04:43:43Z

src/boot/StoreHydratedGate.js

+    () => {
+      init();
+    },


I see, yeah. Here we're happy for it to be an async thing we don't wait for, but it is good to have something making that explicit. Which this does, a little bit.

gnprice · 2022-04-05T04:46:28Z

src/events/eventActions.js

+  type: DEAD_QUEUE,
+});
+
+export const deadQueue = (): ThunkAction<Promise<void>> => async (dispatch, getState) => {


This can still be an unexported const deadQueue when it becomes a thunk action.

gnprice · 2022-04-05T04:50:49Z

src/boot/AppDataFetcher.js

+    // We just want to init once, just after the store rehydrates. This
+    // component will have begun its life at that time, since it's under
+    // HideIfNotHydrated.
+    //
+    // eslint-disable-next-line react-hooks/exhaustive-deps
+    [],


I think this still calls for an explanation of why it's OK that we only call this once, even if hasAuth later changes. Along the lines of here:
#5300 (comment)

gnprice · 2022-04-05T04:58:22Z

src/events/eventActions.js

-  // TODO: Remove this. No need to do this on DEAD_QUEUE or ACCOUNT_SWITCH:
-  //   https://chat.zulip.org/#narrow/stream/48-mobile/topic/.60remoteNotificationsRegistered.60.20listener.20too.20late/near/1148915
-  dispatch(initNotifications());


I think this is the description of what the relevant subsystem is aimed at:

Yeah, that all seems right.

Here's an example of how the code is hard to follow. It occurred to me after your comment prompted me to keep thinking about this: […]

Our "on-startup" code for this is initNotifications. That sure doesn't look like "some global code", does it? It's a ThunkAction, not a GlobalThunkAction. But then, if you sit and think about it and read some distant code, it's possible to convince yourself that all interested servers will indeed be notified if the device token changed between sessions of running the app, on iOS and Android.

Hmm, indeed. I agree, that seems like a key point for why this logic is hard to follow.

Want to make an issue for this? That'll probably provide a better way to track it than this review subthread.

gnprice · 2022-04-05T04:59:02Z

Thanks! This generally looks good; small comments above.

To replace useEdgeTriggeredEffect, which implicitly required a constant callback; see zulip#5300 (comment) and this bit of useEdgeTriggeredEffect's jsdoc: > The callback is not permitted to return a cleanup function, > because it's not clear what the semantics should be of when such a > cleanup function would be run.

Following Greg's explanation at zulip#5300 (comment)

chrisbobbe · 2022-04-07T22:24:37Z

Thanks for the review! Revision pushed.

Following Greg's explanation at zulip#5300 (comment)

chrisbobbe · 2022-06-24T22:44:15Z

Just rebased (whew, there was a lot to do this time, especially after #5393!)

gnprice · 2022-06-25T01:02:38Z

Just rebased (whew, there was a lot to do this time, especially after #5393 [the Prettier 2 upgrade]!)

Hmm, yeah -- we didn't discuss workflow for open PRs, did we. For comparable changes in zulip/zulip, there's generally an explicit set of steps pre-developed, tested, and then announced to contributors, for how to rebase their work past the change.

I think the basic algorithm that should reliably work, and avoids a lot of manual conflict resolution, is:

Rebase onto the commit just before the formatting change. Resolve conflicts along the way as usual.
Rebase onto the commit that made the formatting change. Resolve each conflict mechanically, by taking your changed code and auto-formatting it: like git checkout --theirs . && tools/test prettier --fix --diff @ && git add -u && git rebase --continue. (Well, and prefixed with yarn install && the first time.)
Rebase onto the latest upstream. Resolve conflicts along the way as usual.

I did something like that for one complex PR I had open at the time, and it worked well. I have plenty of draft branches I'll probably have to do something similar with, and you likely do too. I guess let's discuss in a chat thread with whatever we find the next time one of us does a rebase that calls for this. Should be possible to distill it into a short copy-pasteable script, and/or a little script in tools/ (though it's a bit tricky to use the latter, since by definition one's acting on old branches.)

chrisbobbe · 2022-06-26T16:52:08Z

Oh, yeah, that's a very good idea. 🙂

This bit stopped being true in 8774806.

Along with its helper logoutPlain. Soon, we'll get rid of the needsInitialFetch system, and call register-queue code directly from thunk actions, including two in accountActions.js: loginSuccess and accountSwitch. Since that register-queue code can end up calling `logout`, and since that code is (and indeed naturally belongs) outside accountActions.js, we'd end up with an import cycle if `logout` stayed in accountActions.js. So, prevent that import cycle from happening by moving `logout` to a new file.

This thunk action includes registering an event queue, of course -- but it has also included some unrelated tasks: notification setup and an invocation of the broken outbox subsystem. Make this action only about registering a queue and starting a polling loop on it, pulling out that other stuff. Also change its name to explicitly say what it does: not just the "initial fetch", which I think basically means POST /register [1], but also starting a polling loop, which should always be done synchronously after that. The fetch-PMs call stays in the function just because it's meant to fill in a shortcoming of the register-queue functionality on older servers. We still plan to remove that call when we desupport server 2.1. Possibly the unrelated stuff should be grouped into a new thunk action of its own, but I don't yet see a nice name or description for such a group. Next, we'll move this and its helpers to eventActions.js. [1] See https://zulip.readthedocs.io/en/latest/subsystems/events-system.html#the-initial-data-fetch . But also "initial" can be a bit misleading, because it could misleadingly imply, for example, that this is only meant to run on startup and on first-time connection with an account. We run this with DEAD_QUEUE and ACCOUNT_SWITCH too, and will probably run it on `restart` events for zulip#4793.

…ions Along with its helpers: - registerStart - registerAbortPlain - registerAbort - registerComplete - registerAndStartPolling (fetchPrivateMessages is also a helper, but would be kind of annoying to uproot, including exporting messageFetchComplete, and it has a TODO(server-2.1) for deletion anyway.) This seems like a better place for these than something with "message" in the path -- the events system deals with a lot more than just messages.

Exporting this doesn't seem necessary, even before this series of commits.

It's basically a helper of startEventPolling (well, and registerAndStartPolling, while it uses deadQueue in a hack.)

We've long thought that the current logic is complex and fragile; see for example https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/Stuck.20on.20loading.20screen/near/907688 To accomplish this, we'll remove each case where `sessionReducer` sets `needsInitialFetch` to `true`, and instead just directly dispatch `registerAndStartPolling` and its two companions (`sendOutbox and `initNotifications`) in the appropriate thunk action as appropriate. Start with LOGIN_SUCCESS.

This time for ACCOUNT_SWITCH: just directly dispatch `registerAndStartPolling` and friends in the thunk action instead of invoking them through `needsInitialFetch` / `AppDataFetcher`.

Following Greg's explanation at zulip#5300 (comment)

This time for DEAD_QUEUE: just directly dispatch `registerAndStartPolling` and friends in the thunk action instead of invoking them through `needsInitialFetch` / `AppDataFetcher`.

Just before this commit, the only time `needsInitialFetch` goes from `false` to `true` -- which is the only time AppDataFetcher does its "initial fetch" -- is upon store rehydration, and only then if `getHasAuth` (on the hydration payload) gives true. Here, we express the same thing, but locally in `AppDataFetcher`, using the fact that this component begins its life upon store rehydration, and using `hasAuth` as selected from Redux. Co-authored-by: Greg Price <gnprice@zulip.com>

At this point, it's just a value sitting in Redux that's always `false`. Remove it, and some documentation that described it.

…singleton It looks like this component was written to be flexible and reusable, with callers giving their own PlaceholderComponent as desired. So far we've only used it once. And that's not a coincidence: encapsulating this check in just one place is a sensible design; see https://github.com/zulip/zulip-mobile/blob/master/docs/architecture/crunchy-shell.md Not doing so, and instead having to check `isHydrated` in multiple places, risks being confusing. So, let this component settle into its role as a singleton.

gnprice

OK, revision reviewed! I also re-read the whole branch to refresh myself on all the changes and make one more review pass on it, given that this is reworking such a core subsystem.

Everything looks good except a few small comments below -- all of which are on the three newer commits at the end. Those are close, but this PR thread has already gotten long for GitHub. So I think a good plan is:

I'll merge the other 15 commits. 🎉
Then please send a revision of those last 3 commits as a fresh PR.

gnprice · 2022-06-25T01:24:50Z

src/boot/StoreProvider.js

+        const dispatch: Dispatch = store.dispatch;
+
+        // Init right away if there's an active, logged-in account.
+        // NB `getInitialRouteInfo` depends intimately on this behavior.


Generally a comment like this one should also have a similar comment at the other end. So seeing such a comment get moved makes me curious about that other end, if it isn't being updated in the same commit. 🙂

Here, that looks like:

// Show the main UI screen. // // If we don't have server data yet, that screen will show a loading // indicator until the data is loaded. Crucially, AppDataFetcher will make // sure we really will be loading. return { initialRouteName: 'main-tabs' };

So that comment should get updated to match.

More broadly, git grep AppDataFetcher finds a handful of other doc and comment references, so each of those should get updated.

gnprice · 2022-06-25T01:28:05Z

src/boot/store.js

@@ -130,6 +130,7 @@ function listMiddleware() {
 *  * docs/architecture/realtime.md
 *  * docs/background/recommended-reading.md
 */
+// TODO: Represent thunk actions, etc.


Let's squash this line into the parent commit, which has this:

+ // The `store` type isn't complete: it ignores thunk actions, etc. + // $FlowFixMe[incompatible-type] + const dispatch: Dispatch = store.dispatch;

That provides a bit more context as to what this is about.

gnprice · 2022-06-25T01:30:53Z

src/boot/StoreProvider.js

-      this.unsubscribeStoreObserver();
-    }
-  }
+    return () => unsubscribeStoreObserver();


nit: this can be simplified to:

Suggested change

return () => unsubscribeStoreObserver();

return unsubscribeStoreObserver;

And then this and the preceding statement can be simplified further, to:

return observeStore( store, // …

I think that last version makes a pretty idiomatic way to write this kind of effect with a cleanup callback.

(The version in this revision makes sense for the "Convert to function component" commit, as it's the most direct translation of the componentDidMount / this.unsubscribeStoreObserver / componentWillUnmount combination. But then it can be cleaned up afterward.)

Another cleanup that becomes possible after the Hooks conversion: break this up into a couple of separate effects. One for the observeStore watching zulipVersion, and one for the restore call and its callback.

That also provides the opportunity to put the observeStore effect first (without having to keep around unsubscribeStoreObserver over a longer stretch of code.) That just makes it a bit easier to look at this code and be sure that the observer is in place in time to see the persisted data when it gets restored.

As Greg suggests: zulip#5300 (comment)

As Greg points out: zulip#5300 (comment) > That just makes it a bit easier to look at this code and be sure > that the observer is in place in time to see the persisted data > when it gets restored.

chrisbobbe requested a review from gnprice March 15, 2022 23:49

gnprice reviewed Mar 17, 2022

View reviewed changes

chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this pull request Mar 22, 2022

notifOpen [nfc]: Make a TODO less terse

8f29469

Following Greg's explanation at zulip#5300 (comment)

chrisbobbe force-pushed the pr-rm-needs-initial-fetch branch from b752b7a to 6cd478e Compare March 22, 2022 19:38

gnprice reviewed Apr 5, 2022

View reviewed changes

chrisbobbe mentioned this pull request Apr 7, 2022

notifs: Token-registration code hard to follow, and different per-platform #5329

Open

chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this pull request Apr 7, 2022

notifOpen [nfc]: Make a TODO less terse

944c862

Following Greg's explanation at zulip#5300 (comment)

chrisbobbe force-pushed the pr-rm-needs-initial-fetch branch from 6cd478e to 3edfe96 Compare April 7, 2022 22:24

chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this pull request Jun 24, 2022

notifOpen [nfc]: Make a TODO less terse

05b9892

Following Greg's explanation at zulip#5300 (comment)

chrisbobbe force-pushed the pr-rm-needs-initial-fetch branch from 3edfe96 to 8f59a4d Compare June 24, 2022 22:43

chrisbobbe added 9 commits June 27, 2022 11:18

registerAbort [nfc]: Cut something false from a comment

e96ee59

This bit stopped being true in 8774806.

doInitialFetch [nfc]: Add TODO on sendOutbox

2006a78

eventActions [nfc]: Unexport registerAbort

0450af1

Exporting this doesn't seem necessary, even before this series of commits.

eventActions [nfc]: Move deadQueue here, from sessionActions

5f70582

It's basically a helper of startEventPolling (well, and registerAndStartPolling, while it uses deadQueue in a hack.)

events [nfc]: Get rid of needsInitialFetch (2/n)

8568d57

This time for ACCOUNT_SWITCH: just directly dispatch `registerAndStartPolling` and friends in the thunk action instead of invoking them through `needsInitialFetch` / `AppDataFetcher`.

chrisbobbe and others added 6 commits June 27, 2022 11:18

notifOpen [nfc]: Make a TODO less terse

a164199

Following Greg's explanation at zulip#5300 (comment)

events [nfc]: Get rid of needsInitialFetch (3/n)

58c3891

This time for DEAD_QUEUE: just directly dispatch `registerAndStartPolling` and friends in the thunk action instead of invoking them through `needsInitialFetch` / `AppDataFetcher`.

events [nfc]: Get rid of needsInitialFetch (5/n)

37e1dd7

At this point, it's just a value sitting in Redux that's always `false`. Remove it, and some documentation that described it.

StoreHydratedGate [nfc]: Tighten up code

c5e0b0c

gnprice reviewed Jun 27, 2022

View reviewed changes

gnprice force-pushed the pr-rm-needs-initial-fetch branch from 8f59a4d to c5e0b0c Compare June 27, 2022 19:20

gnprice merged commit c5e0b0c into zulip:main Jun 27, 2022

chrisbobbe deleted the pr-rm-needs-initial-fetch branch July 7, 2022 23:49

chrisbobbe restored the pr-rm-needs-initial-fetch branch July 7, 2022 23:49

chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this pull request Jul 8, 2022

StoreProvider [nfc]: Split useEffect into two useEffects

2bf9a0e

As Greg suggests: zulip#5300 (comment)

chrisbobbe deleted the pr-rm-needs-initial-fetch branch July 8, 2022 17:29

chrisbobbe mentioned this pull request Jul 8, 2022

Followup to removing brittle needsInitialFetch logic #5438

Merged

		* Also do some miscellaneous other work we want to do when starting
		* up, or regaining a network connection. We fetch private messages

	return () => unsubscribeStoreObserver();
	return unsubscribeStoreObserver;

events: Remove brittle needsInitialFetch system #5300

events: Remove brittle needsInitialFetch system #5300

Conversation

chrisbobbe commented Mar 15, 2022 • edited Loading

gnprice left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisbobbe Mar 21, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisbobbe Mar 22, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisbobbe commented Mar 22, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gnprice commented Apr 5, 2022

chrisbobbe commented Apr 7, 2022

chrisbobbe commented Jun 24, 2022

gnprice commented Jun 25, 2022

chrisbobbe commented Jun 26, 2022

gnprice left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

events: Remove brittle `needsInitialFetch` system #5300

events: Remove brittle `needsInitialFetch` system #5300

chrisbobbe commented Mar 15, 2022 •

edited

Loading

chrisbobbe Mar 21, 2022 •

edited

Loading

chrisbobbe Mar 22, 2022 •

edited

Loading