Skip to content

Commit

Permalink
fetchActions: Use loading instead of needsInitialFetch.
Browse files Browse the repository at this point in the history
`needsInitialFetch` really belongs to the brittle `AppDataFetcher`
subsystem, and we've long wanted to replace that with something more
robust, to reduce the amount of nonlocal reasoning we have to do in
the initial fetch logic.

It seems likely that the old code was trying to say, "Don't do a
`fetchOlder` or `fetchNewer` when an initial fetch is in progress".
And `loading` is a better way of saying "an initial fetch is in
progress".

Greg summarizes why we shouldn't expect this to change behavior in a
disruptive way [1]:

> The key points are:
>
> * `needsInitialFetch` is never false when `loading` is true
> * whenever `needsInitialFetch` is true and `loading` isn't,
>   `AppDataFetcher` will immediately cause `loading` to become true
> * so at most, this means
>   > we pretty minimally extend the existing window during which
>   > it's possible for this code to start a message fetch that ends
>   > up still being in progress when an initial fetch starts.
>
> It's a bug, and a potential source of wrong data, that that window
> exists -- or rather, that we don't cancel that earlier message
> fetch and may use its results, which are potentially missing
> updates that happened before we created our new event queue. But
> this change doesn't meaningfully expand that bug, and it helps us
> toward simplifying the `needsInitialFetch` / `AppDataFetcher`
> logic.

[1] zulip#5003 (comment)
  • Loading branch information
chrisbobbe authored and gnprice committed Sep 30, 2021
1 parent a71f164 commit 8774806
Show file tree
Hide file tree
Showing 2 changed files with 8 additions and 8 deletions.
8 changes: 4 additions & 4 deletions src/message/__tests__/fetchActions-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -566,12 +566,12 @@ describe('fetchActions', () => {
expect(actions).toHaveLength(0);
});

test('when needsInitialFetch is true, no action is dispatched', async () => {
test('when loading is true, no action is dispatched', async () => {
const store = mockStore<GlobalState, Action>({
...baseState,
session: {
...baseState.session,
needsInitialFetch: true,
loading: true,
},
});

Expand Down Expand Up @@ -658,12 +658,12 @@ describe('fetchActions', () => {
expect(actions).toHaveLength(0);
});

test('when needsInitialFetch is true, no action is dispatched', async () => {
test('when loading is true, no action is dispatched', async () => {
const store = mockStore<GlobalState, Action>({
...baseState,
session: {
...baseState.session,
needsInitialFetch: true,
loading: true,
},
});

Expand Down
8 changes: 4 additions & 4 deletions src/message/fetchActions.js
Original file line number Diff line number Diff line change
Expand Up @@ -160,9 +160,9 @@ export const fetchOlder = (narrow: Narrow): ThunkAction<void> => (dispatch, getS
const firstMessageId = getFirstMessageId(state, narrow);
const caughtUp = getCaughtUpForNarrow(state, narrow);
const fetching = getFetchingForNarrow(state, narrow);
const { needsInitialFetch } = getSession(state);
const { loading } = getSession(state);

if (!needsInitialFetch && !fetching.older && !caughtUp.older && firstMessageId !== undefined) {
if (!loading && !fetching.older && !caughtUp.older && firstMessageId !== undefined) {
dispatch(
fetchMessages({
narrow,
Expand All @@ -179,9 +179,9 @@ export const fetchNewer = (narrow: Narrow): ThunkAction<void> => (dispatch, getS
const lastMessageId = getLastMessageId(state, narrow);
const caughtUp = getCaughtUpForNarrow(state, narrow);
const fetching = getFetchingForNarrow(state, narrow);
const { needsInitialFetch } = getSession(state);
const { loading } = getSession(state);

if (!needsInitialFetch && !fetching.newer && !caughtUp.newer && lastMessageId !== undefined) {
if (!loading && !fetching.newer && !caughtUp.newer && lastMessageId !== undefined) {
dispatch(
fetchMessages({
narrow,
Expand Down

0 comments on commit 8774806

Please sign in to comment.