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

refactor useQuery to not use an internal class #11869

Merged
merged 33 commits into from
Jul 4, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
33 commits
Select commit Hold shift + click to select a range
c2bd48b
extract hooks from InternalState
phryneas May 27, 2024
dc58beb
inline hooks and functions into each other, move `createWatchQueryOpt…
phryneas May 27, 2024
10466e5
move `executeQuery` to `useLazyQuery`
phryneas May 27, 2024
ea77f60
move more functions out
phryneas May 27, 2024
fde4c4d
move `unsafeHandlePartialRefetch` into `setResult`
phryneas May 28, 2024
e1b7ed7
remove `toQueryResultCache`, save transformed result in `internalStat…
phryneas May 29, 2024
9c865ca
fixup test
phryneas May 29, 2024
b5e1643
move `getDefaultFetchPolicy` out
phryneas May 29, 2024
b4c8bf9
move more functions out
phryneas May 29, 2024
12e19f9
moved all class methods out
phryneas May 29, 2024
24e4491
replace class with single mutable object
phryneas May 29, 2024
e31d953
move callbacks into their own ref
phryneas May 29, 2024
891e211
move `obsQueryFields` out of `internalState`
phryneas Jun 3, 2024
ec1c7a9
inline `useInternalState`
phryneas Jun 4, 2024
79566a8
redactor away `internalState.queryHookOptions`
phryneas Jun 4, 2024
873f24d
make function arguments more explicit
phryneas Jun 4, 2024
8bb445c
replace `internalState.watchQueryOptions` with `observable[lastWatchO…
phryneas Jun 5, 2024
635a32b
move observable fully into a readonly prop on internalState
phryneas Jun 5, 2024
d6de17f
remove all direct access to `internalState` after initializing
phryneas Jun 5, 2024
06d98ac
extract new `useInternalState` hook
phryneas Jun 5, 2024
7717b4f
extract `useResubscribeIfNecessary` hook
phryneas Jun 5, 2024
3889fff
add comment
phryneas Jun 5, 2024
da4b840
extract `bindObservableMethods`
phryneas Jun 7, 2024
f18b753
extract `useHandleSkip` and `useRegisterSSRObservable` hooks
phryneas Jun 7, 2024
8480ce6
extract useObservableSubscriptionResult hook
phryneas Jun 7, 2024
30b1769
change some method arguments. remove obsolete comment
phryneas Jun 7, 2024
c896885
curry `createMakeWatchQueryOptions`
phryneas Jun 7, 2024
1485651
bring function and hook argyuments into a common order
phryneas Jun 7, 2024
70f5aaf
Move `onQueryExecuted` into `useInternalState`
phryneas Jun 7, 2024
fed117b
some style adjustments to be more compiler-friendly
phryneas Jun 7, 2024
a69327c
remove R19 exception from test, chores
phryneas Jul 3, 2024
987aaad
Apply suggestions from code review
phryneas Jul 4, 2024
33c0fef
Merge branch 'release-3.11' into pr/rewrite-useQuery
phryneas Jul 4, 2024
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
5 changes: 5 additions & 0 deletions .changeset/nasty-olives-act.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@apollo/client": minor
---

Rewrite big parts of `useQuery` and `useLazyQuery` to be more compliant with the Rules of React and React Compiler
4 changes: 2 additions & 2 deletions .size-limits.json
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
{
"dist/apollo-client.min.cjs": 39619,
"import { ApolloClient, InMemoryCache, HttpLink } from \"dist/index.js\" (production)": 32852
"dist/apollo-client.min.cjs": 39825,
"import { ApolloClient, InMemoryCache, HttpLink } from \"dist/index.js\" (production)": 32851
}
29 changes: 1 addition & 28 deletions src/react/hooks/__tests__/useQuery.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,6 @@ import { useApolloClient } from "../useApolloClient";
import { useLazyQuery } from "../useLazyQuery";

const IS_REACT_17 = React.version.startsWith("17");
const IS_REACT_19 = React.version.startsWith("19");

describe("useQuery Hook", () => {
describe("General use", () => {
Expand Down Expand Up @@ -1536,33 +1535,7 @@ describe("useQuery Hook", () => {

function checkObservableQueries(expectedLinkCount: number) {
const obsQueries = client.getObservableQueries("all");
/*
This is due to a timing change in React 19

In React 18, you observe this pattern:

1. render
2. useState initializer
3. component continues to render with first state
4. strictMode: render again
5. strictMode: call useState initializer again
6. component continues to render with second state

now, in React 19 it looks like this:

1. render
2. useState initializer
3. strictMode: call useState initializer again
4. component continues to render with one of these two states
5. strictMode: render again
6. component continues to render with the same state as during the first render

Since useQuery breaks the rules of React and mutably creates an ObservableQuery on the state during render if none is present, React 18 did create two, while React 19 only creates one.

This is pure coincidence though, and the useQuery rewrite that doesn't break the rules of hooks as much and creates the ObservableQuery as part of the state initializer will end up with behaviour closer to the old React 18 behaviour again.

*/
expect(obsQueries.size).toBe(IS_REACT_19 ? 1 : 2);
expect(obsQueries.size).toBe(2);
Copy link
Member

Choose a reason for hiding this comment

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

🎉 🎉 🎉


const activeSet = new Set<typeof result.current.observable>();
const inactiveSet = new Set<typeof result.current.observable>();
Expand Down
136 changes: 117 additions & 19 deletions src/react/hooks/useLazyQuery.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,16 +2,30 @@ import type { DocumentNode } from "graphql";
import type { TypedDocumentNode } from "@graphql-typed-document-node/core";
import * as React from "rehackt";

import type { OperationVariables } from "../../core/index.js";
import type {
ApolloClient,
ApolloQueryResult,
OperationVariables,
WatchQueryOptions,
} from "../../core/index.js";
import { mergeOptions } from "../../utilities/index.js";
import type {
LazyQueryHookExecOptions,
LazyQueryHookOptions,
LazyQueryResultTuple,
NoInfer,
QueryHookOptions,
QueryResult,
} from "../types/types.js";
import { useInternalState } from "./useQuery.js";
import { useApolloClient } from "./useApolloClient.js";
import type { InternalResult, ObsQueryWithMeta } from "./useQuery.js";
import {
createMakeWatchQueryOptions,
getDefaultFetchPolicy,
getObsQueryOptions,
toQueryResult,
useQueryInternals,
} from "./useQuery.js";
import { useIsomorphicLayoutEffect } from "./internal/useIsomorphicLayoutEffect.js";

// The following methods, when called will execute the query, regardless of
// whether the useLazyQuery execute function was called before.
Expand All @@ -21,6 +35,7 @@ const EAGER_METHODS = [
"fetchMore",
"updateQuery",
"startPolling",
"stopPolling",
"subscribeToMore",
] as const;

Expand Down Expand Up @@ -80,21 +95,27 @@ export function useLazyQuery<
optionsRef.current = options;
queryRef.current = document;

const internalState = useInternalState<TData, TVariables>(
useApolloClient(options && options.client),
document
);

const useQueryResult = internalState.useQuery({
const queryHookOptions = {
...merged,
skip: !execOptionsRef.current,
});
};
const {
obsQueryFields,
result: useQueryResult,
client,
resultData,
observable,
onQueryExecuted,
} = useQueryInternals(document, queryHookOptions);

const initialFetchPolicy =
useQueryResult.observable.options.initialFetchPolicy ||
internalState.getDefaultFetchPolicy();
observable.options.initialFetchPolicy ||
getDefaultFetchPolicy(
queryHookOptions.defaultOptions,
client.defaultOptions
);

const { forceUpdateState, obsQueryFields } = internalState;
const forceUpdateState = React.useReducer((tick) => tick + 1, 0)[1];
// We use useMemo here to make sure the eager methods have a stable identity.
const eagerMethods = React.useMemo(() => {
const eagerMethods: Record<string, any> = {};
Expand All @@ -111,7 +132,7 @@ export function useLazyQuery<
};
}

return eagerMethods;
return eagerMethods as typeof obsQueryFields;
}, [forceUpdateState, obsQueryFields]);

const called = !!execOptionsRef.current;
Expand Down Expand Up @@ -141,18 +162,95 @@ export function useLazyQuery<
...execOptionsRef.current,
});

const promise = internalState
.executeQuery({ ...options, skip: false })
.then((queryResult) => Object.assign(queryResult, eagerMethods));
const promise = executeQuery(
resultData,
observable,
client,
document,
{ ...options, skip: false },
onQueryExecuted
).then((queryResult) => Object.assign(queryResult, eagerMethods));

// Because the return value of `useLazyQuery` is usually floated, we need
// to catch the promise to prevent unhandled rejections.
promise.catch(() => {});

return promise;
},
[eagerMethods, initialFetchPolicy, internalState]
[
client,
document,
eagerMethods,
initialFetchPolicy,
observable,
resultData,
onQueryExecuted,
]
);

const executeRef = React.useRef(execute);
useIsomorphicLayoutEffect(() => {
executeRef.current = execute;
});

const stableExecute = React.useCallback<typeof execute>(
(...args) => executeRef.current(...args),
[]
);
return [stableExecute, result];
}

return [execute, result];
function executeQuery<TData, TVariables extends OperationVariables>(
resultData: InternalResult<TData, TVariables>,
observable: ObsQueryWithMeta<TData, TVariables>,
client: ApolloClient<object>,
currentQuery: DocumentNode,
options: QueryHookOptions<TData, TVariables> & {
query?: DocumentNode;
},
onQueryExecuted: (options: WatchQueryOptions<TVariables, TData>) => void
) {
const query = options.query || currentQuery;
const watchQueryOptions = createMakeWatchQueryOptions(
client,
query,
options,
false
)(observable);

const concast = observable.reobserveAsConcast(
getObsQueryOptions(observable, client, options, watchQueryOptions)
);
onQueryExecuted(watchQueryOptions);

return new Promise<
Omit<QueryResult<TData, TVariables>, (typeof EAGER_METHODS)[number]>
>((resolve) => {
let result: ApolloQueryResult<TData>;

// Subscribe to the concast independently of the ObservableQuery in case
// the component gets unmounted before the promise resolves. This prevents
// the concast from terminating early and resolving with `undefined` when
// there are no more subscribers for the concast.
concast.subscribe({
next: (value) => {
result = value;
},
error: () => {
resolve(
toQueryResult(
observable.getCurrentResult(),
resultData.previousData,
observable,
client
)
);
},
complete: () => {
resolve(
toQueryResult(result, resultData.previousData, observable, client)
);
},
});
});
}
Loading
Loading