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 15 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
96 changes: 71 additions & 25 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,48 @@ 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 QueryRefPromise<TData> = PromiseWithState<ApolloQueryResult<TData>> &
WithSequence;

type Listener<TData> = (promise: Promise<ApolloQueryResult<TData>>) => void;
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();

function createSequencedFulfilledPromise<TValue>(value: TValue) {
return withSequence(createFulfilledPromise(value));
}

function createSequencedRejectedPromise<TValue = unknown>(reason: unknown) {
return withSequence(createRejectedPromise<TValue>(reason));
}

/**
* 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 +58,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 +104,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 +130,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 @@ -103,13 +140,17 @@ export class InternalQueryReference<TData = unknown> {
(this.result.data &&
(!this.result.partial || this.watchQueryOptions.returnPartialData))
) {
this.promise = createFulfilledPromise(this.result);
this.promise = createSequencedFulfilledPromise(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 @@ -189,7 +230,7 @@ export class InternalQueryReference<TData = unknown> {

if (currentCanonizeResults !== watchQueryOptions.canonizeResults) {
this.result = { ...this.result, ...this.observable.getCurrentResult() };
this.promise = createFulfilledPromise(this.result);
this.promise = createSequencedFulfilledPromise(this.result);
}
}

Expand Down Expand Up @@ -249,7 +290,7 @@ export class InternalQueryReference<TData = unknown> {
}

this.result = result;
this.promise = createFulfilledPromise(result);
this.promise = createSequencedFulfilledPromise(result);
this.deliver(this.promise);
break;
}
Expand All @@ -270,23 +311,28 @@ export class InternalQueryReference<TData = unknown> {
break;
}
case "idle": {
this.promise = createRejectedPromise(error);
this.promise =
createSequencedRejectedPromise<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;
}
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
Loading
Loading