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 14 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/khaki-months-rhyme.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@apollo/client": patch
---

ObservableQuery.getCurrentResult: skip the cache if the running query should not access the cache
3 changes: 2 additions & 1 deletion .git-blame-ignore-revs
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
# format "ObservableQuery" test
0a67647b73abd94b706242f32b88d21a1400ad50
# format "ObservableQuery" test (in #10597)
104bf11765b1db50292f9656aa8fe48e2d749a83

# Format "DisplayClientError.js" (#10909)
0cb68364f2c3828badde8c74de44e9c1864fb6d4
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.

}
2 changes: 1 addition & 1 deletion config/bundlesize.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ import { join } from "path";
import { gzipSync } from "zlib";
import bytes from "bytes";

const gzipBundleByteLengthLimit = bytes("33.02KB");
const gzipBundleByteLengthLimit = bytes("33.07KB");
const minFile = join("dist", "apollo-client.min.cjs");
const minPath = join(__dirname, "..", minFile);
const gzipByteLen = gzipSync(readFileSync(minPath)).byteLength;
Expand Down
27 changes: 23 additions & 4 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,16 +243,19 @@ 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) ||
// 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
// return a result synchronously).
this.queryManager.transform(this.options.query).hasForcedResolvers
) {
// Fall through.
// Fall through.
} else if (this.waitForOwnResult) {
// This would usually be a part of `QueryInfo.getDiff()`.
// which we skip in the waitForOwnResult case since we are not
// interested in the diff.
this.queryInfo['updateWatch']();
phryneas marked this conversation as resolved.
Show resolved Hide resolved
} else {
const diff = this.queryInfo.getDiff();

Expand Down Expand Up @@ -833,13 +839,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, fromLink } = 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 @@ -990,3 +1005,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).

}