Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ObservableQuery.getCurrentResult: skip the cache #10631

Merged
merged 17 commits into from
Jun 22, 2023
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion .vscode/settings.json
Original file line number Diff line number Diff line change
Expand Up @@ -7,5 +7,6 @@
"typescript.tsdk": "node_modules/typescript/lib",
"cSpell.enableFiletypes": [
"mdx"
]
],
"jest.jestCommandLine": "node_modules/.bin/jest --config ./config/jest.config.js --ignoreProjects 'ReactDOM 17' --runInBand",
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had to add this with some commit - the Jest VSCode plugin cannot display it when two projects run the same test twice, so I just set it to exclude the React17 tests. Also adds --runInBand for the extension, which makes tests less flaky in general.

}
21 changes: 18 additions & 3 deletions src/core/ObservableQuery.ts
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ import {
FetchMoreQueryOptions,
SubscribeToMoreOptions,
NextFetchPolicyContext,
WatchQueryFetchPolicy,
} from './watchQueryOptions';
import { QueryInfo } from './QueryInfo';
import { MissingFieldError } from '../cache';
Expand Down Expand Up @@ -86,6 +87,7 @@ export class ObservableQuery<
private observers = new Set<Observer<ApolloQueryResult<TData>>>();
private subscriptions = new Set<ObservableSubscription>();

private waitForOwnResult: boolean;
private last?: Last<TData, TVariables>;

private queryInfo: QueryInfo;
Expand Down Expand Up @@ -152,6 +154,7 @@ export class ObservableQuery<
this.queryManager = queryManager;

// active state
this.waitForOwnResult = skipCacheDataFor(options.fetchPolicy);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This has to be initialized in the constructor, as getCurrentResult() will be called before the ObservableQuery is subscribed to for the first time.

this.isTornDown = false;

const {
Expand Down Expand Up @@ -240,9 +243,8 @@ export class ObservableQuery<
if (
// These fetch policies should never deliver data from the cache, unless
// redelivering a previously delivered result.
fetchPolicy === 'network-only' ||
fetchPolicy === 'no-cache' ||
fetchPolicy === 'standby' ||
skipCacheDataFor(fetchPolicy) ||
this.waitForOwnResult ||
// If this.options.query has @client(always: true) fields, we cannot
// trust diff.result, since it was read from the cache without running
// local resolvers (and it's too late to run resolvers now, since we must
Expand Down Expand Up @@ -834,13 +836,22 @@ Did you mean to call refetch(variables) instead of refetch({ variables })?`);
}
}

this.waitForOwnResult &&= skipCacheDataFor(options.fetchPolicy)
const finishWaitingForOwnResult = () => {
if (this.concast === concast) {
this.waitForOwnResult = false
}
}
phryneas marked this conversation as resolved.
Show resolved Hide resolved

const variables = options.variables && { ...options.variables };
const concast = this.fetch(options, newNetworkStatus);
const observer: Observer<ApolloQueryResult<TData>> = {
next: result => {
finishWaitingForOwnResult();
this.reportResult(result, variables);
},
error: error => {
finishWaitingForOwnResult();
this.reportError(error, variables);
},
};
Expand Down Expand Up @@ -984,3 +995,7 @@ export function logMissingFieldErrors(
}`, missing);
}
}

function skipCacheDataFor(fetchPolicy?: WatchQueryFetchPolicy) {
phryneas marked this conversation as resolved.
Show resolved Hide resolved
return fetchPolicy === "network-only" || fetchPolicy === "no-cache" || fetchPolicy === "standby";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be worth a comment that when typeof fetchPolicy === "undefined", that means cache-first by default, so skipCacheDataFor should return false (as it does already).

}
2 changes: 1 addition & 1 deletion src/react/hoc/__tests__/queries/loading.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -421,7 +421,7 @@ describe('[queries] loading', () => {
]);
}, { interval: 1 });
await waitFor(() => {
expect(count).toBe(6);
expect(count).toBe(5);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This now rerenders one less time. I checked it - render 5 and 6 had identical this.props.data, so this is probably a good thing:tm:?

}, { interval: 1 });
});

Expand Down
88 changes: 83 additions & 5 deletions src/react/hooks/__tests__/useQuery.test.tsx
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import React, { Fragment, useEffect, useState } from 'react';
import React, { Fragment, ReactNode, useEffect, useState } from 'react';
import { DocumentNode, GraphQLError } from 'graphql';
import gql from 'graphql-tag';
import { act } from 'react-dom/test-utils';
Expand Down Expand Up @@ -1758,14 +1758,14 @@ describe('useQuery Hook', () => {
expect(result.current.data).toEqual({ hello: "world 1" });
}, { interval: 1 });
expect(result.current.loading).toBe(false);


await waitFor(() => {
expect(result.current.data).toEqual({ hello: "world 2" });
}, { interval: 1 });
expect(result.current.loading).toBe(false);


result.current.stopPolling();

await expect(waitFor(() => {
Expand Down Expand Up @@ -5941,6 +5941,84 @@ describe('useQuery Hook', () => {
});
});

// https://github.com/apollographql/apollo-client/issues/10222
describe('regression test issue #10222', () => {
it('maintains initial fetch policy when component unmounts and remounts', async () => {
let helloCount = 1;
const query = gql`{ hello }`;
const link = new ApolloLink(() => {
return new Observable(observer => {
const timer = setTimeout(() => {
observer.next({ data: { hello: `hello ${helloCount++}` } });
observer.complete();
}, 50);

return () => {
clearTimeout(timer);
}
})
})

const cache = new InMemoryCache();

const client = new ApolloClient({
link,
cache
});

let setShow: Function
const Toggler = ({ children }: { children: ReactNode }) => {
const [show, _setShow] = useState(true);
setShow = _setShow;

return show ? <>{children}</> : null;
}

const { result } = renderHook(
() => useQuery(query, {
fetchPolicy: 'network-only',
nextFetchPolicy: 'cache-first'
}),
{
wrapper: ({ children }) => (
<ApolloProvider client={client}>
<Toggler>{children}</Toggler>
</ApolloProvider>
),
},
);

expect(result.current.loading).toBe(true);
expect(result.current.data).toBeUndefined();

await waitFor(() => {
expect(result.current.loading).toBe(false);
});

expect(result.current.data).toEqual({ hello: 'hello 1' });
expect(cache.readQuery({ query })).toEqual({ hello: 'hello 1' })

act(() => {
setShow(false);
});

act(() => {
setShow(true);
});

expect(result.current.loading).toBe(true);
expect(result.current.data).toBeUndefined();

await waitFor(() => {
expect(result.current.loading).toBe(false);
})

expect(result.current.data).toEqual({ hello: 'hello 2' });
expect(cache.readQuery({ query })).toEqual({ hello: 'hello 2' })
});
});


describe('defer', () => {
it('should handle deferred queries', async () => {
const query = gql`
Expand Down Expand Up @@ -6985,7 +7063,7 @@ describe('useQuery Hook', () => {
expect(requestSpy).toHaveBeenCalledTimes(shouldFetchOnFirstRender ? 1 : 0);

// We need to wait a moment before the rerender for everything to settle down.
// This part is unfortunately bound to be flaky - but in some cases there is
// This part is unfortunately bound to be flaky - but in some cases there is
// just nothing to "wait for", except "a moment".
await act(() => new Promise((resolve) => setTimeout(resolve, 10)));

Expand Down