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 9 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
80 changes: 58 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,39 @@ import type {
WatchQueryOptions,
} from "../../core/index.js";
import { isNetworkRequestSettled } from "../../core/index.js";
import type { ObservableSubscription } from "../../utilities/index.js";
import type {
ObservableSubscription,
PromiseWithState,
WithSequence,
} 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 { withSequence, wrapPromiseWithState } from "../../utilities/index.js";
import { secondIfNewerFulfilledOrFirst } from "../../utilities/promises/decoration.js";

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

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 +49,34 @@ 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 reference = queryRef[QUERY_REFERENCE_SYMBOL];
return [
reference,
() =>
secondIfNewerFulfilledOrFirst(
Copy link
Member

@jerelmiller jerelmiller Nov 30, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I know I approved this already, but wanted to throw an alternate idea out here.

I think we can avoid the sequencing altogether by just checking to see if the promises are different from each other. If the reference.promise has changed, it means its been updated to a newer promise. In that way, you can then compare to see if the second has been fulfilled or not.

I just tried this change out with these tests and all continue to pass:

  return [
    reference,
    () => {
      const first = queryRef[PROMISE_SYMBOL];
      const second = reference.promise;

      if (first === second) {
        return first;
      }

      return second.status === "fulfilled" ? second : first;
    },
  ];

Returning first makes the tests fail like you described in my other comment.

How do you feel about this and removing the need for withSequence?

queryRef[PROMISE_SYMBOL],
reference.promise
),
];
}

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

const OBSERVED_CHANGED_OPTIONS = [
Expand All @@ -65,11 +95,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 +121,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 +134,14 @@ 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 = withSequence(
wrapPromiseWithState(
new Promise((resolve, reject) => {
this.resolve = resolve;
this.reject = reject;
})
)
);
}

this.subscription = observable
Expand Down Expand Up @@ -270,23 +302,27 @@ 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 = withSequence(
wrapPromiseWithState(
Copy link
Member Author

Choose a reason for hiding this comment

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

We need to wrap them here, so secondIfNewerFulfilledOrFirst can know if the promise is already fulfilled.

That has a neat side effect:

Wrapping the promises with state here will save React a cycle of "start rendering" -> "suspend to wait for thenable because we don't know that it's already fulfilled" -> "resume rendering".

Async work becomes sync.

Copy link
Member

Choose a reason for hiding this comment

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

ya good call!

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;
}
36 changes: 16 additions & 20 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 @@ -641,7 +641,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 +678,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 +719,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 +779,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 +840,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 +882,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 +922,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 +969,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 +1009,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 +1052,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 @@ -3184,13 +3184,9 @@ describe("useBackgroundQuery", () => {
const user = userEvent.setup();
await act(() => user.click(button));

{
// parent component re-suspends
const { snapshot } = await ProfiledApp.takeRender();
expect(snapshot.suspenseCount).toBe(2);
}
{
const { snapshot, withinDOM } = await ProfiledApp.takeRender();
expect(snapshot.suspenseCount).toBe(1);
// @jerelmiller can you please verify that this is still in the spirit of the test?
// This seems to have moved onto the next render - or before the test skipped one.
expect(snapshot.count).toBe(2);
Expand Down Expand Up @@ -3258,8 +3254,8 @@ describe("useBackgroundQuery", () => {
await screen.findByText("2 - Captain America")
).toBeInTheDocument();

// parent component re-suspends
expect(renders.suspenseCount).toBe(2);
// parent component didn't re-suspend
expect(renders.suspenseCount).toBe(1);
expect(renders.count).toBe(3);

// extra render puts an additional frame into the array
Expand Down Expand Up @@ -3295,8 +3291,8 @@ describe("useBackgroundQuery", () => {
const user = userEvent.setup();
await act(() => user.click(button));

// parent component re-suspends
expect(renders.suspenseCount).toBe(2);
// parent component didn't re-suspend
expect(renders.suspenseCount).toBe(1);
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't seem quite right to me. Refetches should cause the component to re-suspend again unless the refetch is wrapped with a startTransition, so I would expect that clicking refetch twice after the initial load, would cause the suspense boundary to be shown 3 times.

expect(renders.count).toBe(2);

expect(
Expand All @@ -3306,7 +3302,7 @@ describe("useBackgroundQuery", () => {
await act(() => user.click(button));

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

expect(
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
34 changes: 15 additions & 19 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,16 @@ export function useBackgroundQuery<
client.watchQuery(watchQueryOptions as WatchQueryOptions<any, any>)
);

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

const [wrapped, setWrappedQueryRef] = React.useState({
current: wrapQueryRef(queryRef, queryRef.promise),
});
Copy link
Member

Choose a reason for hiding this comment

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

I'm curious about the decision to create this as an object with a current property. It doesn't look like you do any mutable update to the current property and exclusively rely on setWrappedQueryRef.

Since wrapQueryRef will return a new object each time, that should be sufficient enough to ensure React won't bail on the render when comparing old and new values. Could you consider simplifying this to just set the wrapped query ref directly?

Suggested change
const [wrapped, setWrappedQueryRef] = React.useState({
current: wrapQueryRef(queryRef, queryRef.promise),
});
const [wrappedQueryRef, setWrappedQueryRef] = React.useState(() =>
wrapQueryRef(queryRef, queryRef.promise)
);

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 think this might be a relic from the time before we had updateWrappedQueryRef and had to create a new one everytime. Removed it and all tests are still green.

if (unwrapQueryRef(wrapped.current)[0] !== queryRef) {
setWrappedQueryRef({ current: wrapQueryRef(queryRef, queryRef.promise) });
}
let wrappedQueryRef = wrapped.current;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
let wrappedQueryRef = wrapped.current;
const wrappedQueryRef = wrapped.current;

Sorry 😅.

Better yet, since you're not reading this value anywhere but the return from this hook, perhaps you can just use wrapped.current there?

Copy link
Member Author

Choose a reason for hiding this comment

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

Also, the differentiation between wrapped and wrappedQueryRef shouldn't be necessary at all anymore - removing it :)

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 +224,7 @@ export function useBackgroundQuery<
(options) => {
const promise = queryRef.fetchMore(options as FetchMoreQueryOptions<any>);

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

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

setPromiseCache((promiseCache) =>
new Map(promiseCache).set(queryRef.key, queryRef.promise)
);
setWrappedQueryRef({ current: 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