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.

Sign up for GitHub

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

useBackgroundQuery: remove promiseCache, work around race condition #11366

Merged
merged 27 commits into from
Dec 1, 2023
Merged

useBackgroundQuery: remove promiseCache, work around race condition #11366

Show file tree
Hide file tree
Changes from 16 commits
Commits
Show all changes
27 commits
Select commit Hold shift + click to select a range
8ecd36f
`useSuspenseQuery`: remove `promiseCache`
phryneas Nov 15, 2023
b8c51af
missed cleanup
phryneas Nov 15, 2023
79577b5
WIP status: useBackgroundQuery
phryneas Nov 15, 2023
1510019
working reimplementation
phryneas Nov 16, 2023
532505a
update `current[0]` correctly
phryneas Nov 16, 2023
50c2149
change `key` to empty object
phryneas Nov 16, 2023
6dd943a
Merge branch 'pr/remove-useSuspenseQuery-promiseCache' into pr/remove…
phryneas Nov 16, 2023
c9c3177
move `secondIfNewerFulfilledOrFirst` call into `unwrapQueryRef`
phryneas Nov 21, 2023
4a5c3e2
remove unused import
phryneas Nov 21, 2023
083af14
PR feedback
phryneas Nov 30, 2023
3da56f0
more PR feedback
phryneas Nov 30, 2023
2cad580
review feedback
phryneas Nov 30, 2023
346e5e8
re-add delay to test
phryneas Nov 30, 2023
fc66f12
add more mock delays
phryneas Nov 30, 2023
1e3297e
Separate withSequence from createFulfilledPromise and createRejectedP…
jerelmiller Nov 30, 2023
a6e421a
Remove need for sequencing on promises by returning newer promise whe…
jerelmiller Dec 1, 2023
53871a1
Merge remote-tracking branch 'origin/release-3.9' into pr/remove-useB…
phryneas Dec 1, 2023
82bd4ae
`useLoadableQuery`: remove `promiseCache`
phryneas Dec 1, 2023
f8e0e05
remove second argument to `wrapQueryRef`
phryneas Dec 1, 2023
10175b5
chores
phryneas Dec 1, 2023
b248d6c
Merge remote-tracking branch 'origin/release-3.9' into pr/remove-useB…
phryneas Dec 1, 2023
4a413c3
fix most tests by adding delays
phryneas Dec 1, 2023
8010b0c
Clean up Prettier, Size-limit, and Api-Extractor
phryneas Dec 1, 2023
c150398
add `skipNonTrackingRenders` to profiler
phryneas Dec 1, 2023
d07aa6a
add comments to "phantom render" tests
phryneas Dec 1, 2023
1eef04f
Merge branch 'release-3.9' into pr/remove-useBackgroundQuery-promiseC…
phryneas Dec 1, 2023
4bde5a0
Clean up Prettier, Size-limit, and Api-Extractor
phryneas Dec 1, 2023
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
77 changes: 55 additions & 22 deletions src/react/cache/QueryReference.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,28 +7,37 @@ import type {
WatchQueryOptions,
} from "../../core/index.js";
import { isNetworkRequestSettled } from "../../core/index.js";
import type { ObservableSubscription } from "../../utilities/index.js";
import type {
ObservableSubscription,
PromiseWithState,
} from "../../utilities/index.js";
import {
createFulfilledPromise,
createRejectedPromise,
} from "../../utilities/index.js";
import type { CacheKey } from "./types.js";
import type { CacheKey, QueryKey } from "./types.js";
import type { useBackgroundQuery, useReadQuery } from "../hooks/index.js";
import { wrapPromiseWithState } from "../../utilities/index.js";

type Listener<TData> = (promise: Promise<ApolloQueryResult<TData>>) => void;
type QueryRefPromise<TData> = PromiseWithState<ApolloQueryResult<TData>>;

type Listener<TData> = (promise: QueryRefPromise<TData>) => void;

type FetchMoreOptions<TData> = Parameters<
ObservableQuery<TData>["fetchMore"]
>[0];

const QUERY_REFERENCE_SYMBOL: unique symbol = Symbol();
const PROMISE_SYMBOL: unique symbol = Symbol();

/**
* A `QueryReference` is an opaque object returned by {@link useBackgroundQuery}.
* A child component reading the `QueryReference` via {@link useReadQuery} will
* suspend until the promise resolves.
*/
export interface QueryReference<TData = unknown> {
[QUERY_REFERENCE_SYMBOL]: InternalQueryReference<TData>;
readonly [QUERY_REFERENCE_SYMBOL]: InternalQueryReference<TData>;
[PROMISE_SYMBOL]: QueryRefPromise<TData>;
}

interface InternalQueryReferenceOptions {
Expand All @@ -38,15 +47,37 @@ interface InternalQueryReferenceOptions {
}

export function wrapQueryRef<TData>(
internalQueryRef: InternalQueryReference<TData>
internalQueryRef: InternalQueryReference<TData>,
promise: QueryRefPromise<TData>
): QueryReference<TData> {
return { [QUERY_REFERENCE_SYMBOL]: internalQueryRef };
return {
[QUERY_REFERENCE_SYMBOL]: internalQueryRef,
[PROMISE_SYMBOL]: promise,
};
}

export function unwrapQueryRef<TData>(
queryRef: QueryReference<TData>
): InternalQueryReference<TData> {
return queryRef[QUERY_REFERENCE_SYMBOL];
): [InternalQueryReference<TData>, () => QueryRefPromise<TData>] {
const internalQueryRef = queryRef[QUERY_REFERENCE_SYMBOL];

return [
internalQueryRef,
() =>
// There is a chance the query ref's promise has been updated in the time
// the original promise had been suspended. In that case, we want to use
// it instead of the older promise which may contain outdated data.
internalQueryRef.promise.status === "fulfilled"
? internalQueryRef.promise
: queryRef[PROMISE_SYMBOL],
];
}

export function updateWrappedQueryRef<TData>(
queryRef: QueryReference<TData>,
promise: QueryRefPromise<TData>
) {
queryRef[PROMISE_SYMBOL] = promise;
}

const OBSERVED_CHANGED_OPTIONS = [
Expand All @@ -65,11 +96,10 @@ type ObservedOptions = Pick<

export class InternalQueryReference<TData = unknown> {
public result: ApolloQueryResult<TData>;
public readonly key: CacheKey;
public readonly key: QueryKey = {};
public readonly observable: ObservableQuery<TData>;

public promiseCache?: Map<CacheKey, Promise<ApolloQueryResult<TData>>>;
public promise: Promise<ApolloQueryResult<TData>>;
public promise: QueryRefPromise<TData>;

private subscription: ObservableSubscription;
private listeners = new Set<Listener<TData>>();
Expand All @@ -92,7 +122,6 @@ export class InternalQueryReference<TData = unknown> {
// Don't save this result as last result to prevent delivery of last result
// when first subscribing
this.result = observable.getCurrentResult(false);
this.key = options.key;

if (options.onDispose) {
this.onDispose = options.onDispose;
Expand All @@ -106,10 +135,12 @@ export class InternalQueryReference<TData = unknown> {
this.promise = createFulfilledPromise(this.result);
this.status = "idle";
} else {
this.promise = new Promise((resolve, reject) => {
this.resolve = resolve;
this.reject = reject;
});
this.promise = wrapPromiseWithState(
new Promise((resolve, reject) => {
this.resolve = resolve;
this.reject = reject;
})
);
}

this.subscription = observable
Expand Down Expand Up @@ -270,23 +301,25 @@ export class InternalQueryReference<TData = unknown> {
break;
}
case "idle": {
this.promise = createRejectedPromise(error);
this.promise = createRejectedPromise<ApolloQueryResult<TData>>(error);
this.deliver(this.promise);
}
}
}

private deliver(promise: Promise<ApolloQueryResult<TData>>) {
private deliver(promise: QueryRefPromise<TData>) {
this.listeners.forEach((listener) => listener(promise));
}

private initiateFetch(returnedPromise: Promise<ApolloQueryResult<TData>>) {
this.status = "loading";

this.promise = new Promise((resolve, reject) => {
this.resolve = resolve;
this.reject = reject;
});
this.promise = wrapPromiseWithState(
new Promise((resolve, reject) => {
this.resolve = resolve;
this.reject = reject;
})
);

this.promise.catch(() => {});

Expand Down
4 changes: 4 additions & 0 deletions src/react/cache/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,3 +5,7 @@ export type CacheKey = [
stringifiedVariables: string,
...queryKey: any[],
];

export interface QueryKey {
__queryKey?: string;
}
29 changes: 17 additions & 12 deletions src/react/hooks/__tests__/useBackgroundQuery.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -216,6 +216,7 @@ function renderVariablesIntegrationTest({
},
},
},
delay: 200,
};
}
);
Expand Down Expand Up @@ -641,7 +642,7 @@ describe("useBackgroundQuery", () => {

const [queryRef] = result.current;

const _result = await unwrapQueryRef(queryRef).promise;
const _result = await unwrapQueryRef(queryRef)[0].promise;
Copy link
Member

Choose a reason for hiding this comment

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

Oof I really gotta update these tests to use the new profiler helpers 😄. Would love to avoid testing against the query refs directly, but understand the change here!


expect(_result).toEqual({
data: { hello: "world 1" },
Expand Down Expand Up @@ -678,7 +679,7 @@ describe("useBackgroundQuery", () => {

const [queryRef] = result.current;

const _result = await unwrapQueryRef(queryRef).promise;
const _result = await unwrapQueryRef(queryRef)[0].promise;

await waitFor(() => {
expect(_result).toEqual({
Expand Down Expand Up @@ -719,7 +720,7 @@ describe("useBackgroundQuery", () => {

const [queryRef] = result.current;

const _result = await unwrapQueryRef(queryRef).promise;
const _result = await unwrapQueryRef(queryRef)[0].promise;

await waitFor(() => {
expect(_result).toMatchObject({
Expand Down Expand Up @@ -779,7 +780,7 @@ describe("useBackgroundQuery", () => {

const [queryRef] = result.current;

const _result = await unwrapQueryRef(queryRef).promise;
const _result = await unwrapQueryRef(queryRef)[0].promise;
const resultSet = new Set(_result.data.results);
const values = Array.from(resultSet).map((item) => item.value);

Expand Down Expand Up @@ -840,7 +841,7 @@ describe("useBackgroundQuery", () => {

const [queryRef] = result.current;

const _result = await unwrapQueryRef(queryRef).promise;
const _result = await unwrapQueryRef(queryRef)[0].promise;
const resultSet = new Set(_result.data.results);
const values = Array.from(resultSet).map((item) => item.value);

Expand Down Expand Up @@ -882,7 +883,7 @@ describe("useBackgroundQuery", () => {

const [queryRef] = result.current;

const _result = await unwrapQueryRef(queryRef).promise;
const _result = await unwrapQueryRef(queryRef)[0].promise;

expect(_result).toEqual({
data: { hello: "from link" },
Expand Down Expand Up @@ -922,7 +923,7 @@ describe("useBackgroundQuery", () => {

const [queryRef] = result.current;

const _result = await unwrapQueryRef(queryRef).promise;
const _result = await unwrapQueryRef(queryRef)[0].promise;

expect(_result).toEqual({
data: { hello: "from cache" },
Expand Down Expand Up @@ -969,7 +970,7 @@ describe("useBackgroundQuery", () => {

const [queryRef] = result.current;

const _result = await unwrapQueryRef(queryRef).promise;
const _result = await unwrapQueryRef(queryRef)[0].promise;

expect(_result).toEqual({
data: { foo: "bar", hello: "from link" },
Expand Down Expand Up @@ -1009,7 +1010,7 @@ describe("useBackgroundQuery", () => {

const [queryRef] = result.current;

const _result = await unwrapQueryRef(queryRef).promise;
const _result = await unwrapQueryRef(queryRef)[0].promise;

expect(_result).toEqual({
data: { hello: "from link" },
Expand Down Expand Up @@ -1052,7 +1053,7 @@ describe("useBackgroundQuery", () => {

const [queryRef] = result.current;

const _result = await unwrapQueryRef(queryRef).promise;
const _result = await unwrapQueryRef(queryRef)[0].promise;

expect(_result).toEqual({
data: { hello: "from link" },
Expand Down Expand Up @@ -3227,12 +3228,14 @@ describe("useBackgroundQuery", () => {
result: {
data: { character: { id: "1", name: "Captain Marvel" } },
},
delay: 200,
},
{
request: { query, variables: { id: "2" } },
result: {
data: { character: { id: "2", name: "Captain America" } },
},
delay: 200,
},
];

Expand Down Expand Up @@ -3297,7 +3300,7 @@ describe("useBackgroundQuery", () => {

// parent component re-suspends
expect(renders.suspenseCount).toBe(2);
expect(renders.count).toBe(2);
expect(renders.count).toBe(1);

expect(
await screen.findByText("1 - Spider-Man (updated)")
Expand All @@ -3307,11 +3310,13 @@ describe("useBackgroundQuery", () => {

// parent component re-suspends
expect(renders.suspenseCount).toBe(3);
expect(renders.count).toBe(3);
expect(renders.count).toBe(2);

expect(
await screen.findByText("1 - Spider-Man (updated again)")
).toBeInTheDocument();

expect(renders.count).toBe(3);
});
it("throws errors when errors are returned after calling `refetch`", async () => {
using _consoleSpy = spyOnConsole("error");
Expand Down
3 changes: 1 addition & 2 deletions src/react/hooks/__tests__/useSuspenseQuery.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3650,8 +3650,7 @@ describe("useSuspenseQuery", () => {
});

await waitFor(() => expect(renders.errorCount).toBe(1));

expect(client.getObservableQueries().size).toBe(0);
await waitFor(() => expect(client.getObservableQueries().size).toBe(0));
});

it('throws network errors when errorPolicy is set to "none"', async () => {
Expand Down
31 changes: 13 additions & 18 deletions src/react/hooks/useBackgroundQuery.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,11 @@ import type {
WatchQueryOptions,
} from "../../core/index.js";
import { useApolloClient } from "./useApolloClient.js";
import { wrapQueryRef } from "../cache/QueryReference.js";
import {
unwrapQueryRef,
updateWrappedQueryRef,
wrapQueryRef,
} from "../cache/QueryReference.js";
import type { QueryReference } from "../cache/QueryReference.js";
import type { BackgroundQueryHookOptions, NoInfer } from "../types/types.js";
import { __use } from "./internal/index.js";
Expand Down Expand Up @@ -202,13 +206,15 @@ export function useBackgroundQuery<
client.watchQuery(watchQueryOptions as WatchQueryOptions<any, any>)
);

const [promiseCache, setPromiseCache] = React.useState(
() => new Map([[queryRef.key, queryRef.promise]])
const [wrappedQueryRef, setWrappedQueryRef] = React.useState(
wrapQueryRef(queryRef, queryRef.promise)
);

if (unwrapQueryRef(wrappedQueryRef)[0] !== queryRef) {
setWrappedQueryRef(wrapQueryRef(queryRef, queryRef.promise));
}
if (queryRef.didChangeOptions(watchQueryOptions)) {
const promise = queryRef.applyOptions(watchQueryOptions);
promiseCache.set(queryRef.key, promise);
updateWrappedQueryRef(wrappedQueryRef, promise);
}

React.useEffect(() => queryRef.retain(), [queryRef]);
Expand All @@ -217,9 +223,7 @@ export function useBackgroundQuery<
(options) => {
const promise = queryRef.fetchMore(options as FetchMoreQueryOptions<any>);

setPromiseCache((promiseCache) =>
new Map(promiseCache).set(queryRef.key, queryRef.promise)
);
setWrappedQueryRef(wrapQueryRef(queryRef, queryRef.promise));

return promise;
},
Expand All @@ -230,22 +234,13 @@ export function useBackgroundQuery<
(variables) => {
const promise = queryRef.refetch(variables);

setPromiseCache((promiseCache) =>
new Map(promiseCache).set(queryRef.key, queryRef.promise)
);
setWrappedQueryRef(wrapQueryRef(queryRef, queryRef.promise));

return promise;
},
[queryRef]
);

queryRef.promiseCache = promiseCache;

const wrappedQueryRef = React.useMemo(
() => wrapQueryRef(queryRef),
[queryRef]
);

return [
didFetchResult.current ? wrappedQueryRef : void 0,
jerelmiller marked this conversation as resolved.
Show resolved Hide resolved
{ fetchMore, refetch },
Expand Down
Loading
Loading