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 1 commit
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
Prev Previous commit
Next Next commit
make function arguments more explicit
  • Loading branch information
phryneas committed Jul 3, 2024
commit 873f24d0688a6ebf65f6a1e3e787b11f8124638d
13 changes: 10 additions & 3 deletions src/react/hooks/useLazyQuery.ts
Original file line number Diff line number Diff line change
Expand Up @@ -197,13 +197,20 @@ function executeQuery<TData, TVariables extends OperationVariables>(
}

internalState.watchQueryOptions = createWatchQueryOptions(
internalState.client,
internalState.query,
options,
internalState,
hasRenderPromises
hasRenderPromises,
internalState.observable
);

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

// Make sure getCurrentResult returns a fresh ApolloQueryResult<TData>,
Expand Down
87 changes: 47 additions & 40 deletions src/react/hooks/useQuery.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import { useSyncExternalStore } from "./useSyncExternalStore.js";
import { equal } from "@wry/equality";

import type {
ApolloClient,
DefaultOptions,
OperationVariables,
WatchQueryFetchPolicy,
Expand Down Expand Up @@ -184,11 +185,14 @@ export function useQueryWithInternalState<
// rather than left uninitialized.
const renderPromises = React.useContext(getApolloContext()).renderPromises;

const watchQueryOptions = createWatchQueryOptions(
options,
internalState,
!!renderPromises
);
const watchQueryOptions: Readonly<WatchQueryOptions<TVariables, TData>> =
createWatchQueryOptions(
internalState.client,
internalState.query,
options,
!!renderPromises,
internalState.observable
);

// Update this.watchQueryOptions, but only when they have changed, which
// allows us to depend on the referential stability of
Expand All @@ -208,7 +212,12 @@ export function useQueryWithInternalState<
// (potentially) kicks off a network request (for example, when the
// variables have changed), which is technically a side-effect.
internalState.observable.reobserve(
getObsQueryOptions(internalState, options)
getObsQueryOptions(
internalState.client,
options,
internalState.watchQueryOptions,
internalState.observable
)
);

// Make sure getCurrentResult returns a fresh ApolloQueryResult<TData>,
Expand Down Expand Up @@ -243,7 +252,12 @@ export function useQueryWithInternalState<
renderPromises.getSSRObservable(internalState.watchQueryOptions)) ||
internalState.observable || // Reuse this.observable if possible (and not SSR)
internalState.client.watchQuery(
getObsQueryOptions(internalState, options)
getObsQueryOptions(
internalState.client,
options,
internalState.watchQueryOptions,
undefined
)
));

const obsQueryFields = React.useMemo<
Expand Down Expand Up @@ -403,6 +417,8 @@ export function createWatchQueryOptions<
TData = any,
TVariables extends OperationVariables = OperationVariables,
>(
client: ApolloClient<object>,
query: DocumentNode | TypedDocumentNode<TData, TVariables>,
{
skip,
ssr,
Expand All @@ -414,14 +430,14 @@ export function createWatchQueryOptions<
// query property that we add below.
...otherOptions
}: QueryHookOptions<TData, TVariables> = {},
internalState: InternalState<TData, TVariables>,
hasRenderPromises: boolean
hasRenderPromises: boolean,
observable: ObservableQuery<TData, TVariables> | undefined
): WatchQueryOptions<TVariables, TData> {
// This Object.assign is safe because otherOptions is a fresh ...rest object
// that did not exist until just now, so modifications are still allowed.
const watchQueryOptions: WatchQueryOptions<TVariables, TData> = Object.assign(
otherOptions,
{ query: internalState.query }
{ query }
);

if (
Expand All @@ -439,28 +455,18 @@ export function createWatchQueryOptions<
}

if (skip) {
const {
fetchPolicy = getDefaultFetchPolicy(
defaultOptions,
internalState.client.defaultOptions
),
initialFetchPolicy = fetchPolicy,
} = watchQueryOptions;

// When skipping, we set watchQueryOptions.fetchPolicy initially to
// "standby", but we also need/want to preserve the initial non-standby
// fetchPolicy that would have been used if not skipping.
Object.assign(watchQueryOptions, {
initialFetchPolicy,
fetchPolicy: "standby",
});
watchQueryOptions.initialFetchPolicy =
watchQueryOptions.initialFetchPolicy ||
watchQueryOptions.fetchPolicy ||
getDefaultFetchPolicy(defaultOptions, client.defaultOptions);
watchQueryOptions.fetchPolicy = "standby";
} else if (!watchQueryOptions.fetchPolicy) {
watchQueryOptions.fetchPolicy =
internalState.observable?.options.initialFetchPolicy ||
getDefaultFetchPolicy(
defaultOptions,
internalState.client.defaultOptions
);
observable?.options.initialFetchPolicy ||
getDefaultFetchPolicy(defaultOptions, client.defaultOptions);
}

return watchQueryOptions;
Expand All @@ -470,12 +476,14 @@ export function getObsQueryOptions<
TData,
TVariables extends OperationVariables,
>(
internalState: InternalState<TData, TVariables>,
queryHookOptions: QueryHookOptions<TData, TVariables>
client: ApolloClient<object>,
queryHookOptions: QueryHookOptions<TData, TVariables>,
watchQueryOptions: Partial<WatchQueryOptions<TVariables, TData>>,
observable: ObservableQuery<TData, TVariables> | undefined
): WatchQueryOptions<TVariables, TData> {
const toMerge: Array<Partial<WatchQueryOptions<TVariables, TData>>> = [];

const globalDefaults = internalState.client.defaultOptions.watchQuery;
const globalDefaults = client.defaultOptions.watchQuery;
if (globalDefaults) toMerge.push(globalDefaults);

if (queryHookOptions.defaultOptions) {
Expand All @@ -492,12 +500,7 @@ export function getObsQueryOptions<
// (if provided) should be merged, to ensure individual defaulted
// variables always have values, if not otherwise defined in
// observable.options or watchQueryOptions.
toMerge.push(
compact(
internalState.observable && internalState.observable.options,
internalState.watchQueryOptions
)
);
toMerge.push(compact(observable && observable.options, watchQueryOptions));

return toMerge.reduce(mergeOptions) as WatchQueryOptions<TVariables, TData>;
}
Expand All @@ -514,7 +517,11 @@ function setResult<TData, TVariables extends OperationVariables>(
internalState.previousData = previousResult.data;
}
internalState.result = toQueryResult(
unsafeHandlePartialRefetch(nextResult, internalState, partialRefetch),
unsafeHandlePartialRefetch(
nextResult,
internalState.observable,
partialRefetch
),
internalState
);
// Calling state.setResult always triggers an update, though some call sites
Expand Down Expand Up @@ -634,7 +641,7 @@ function unsafeHandlePartialRefetch<
TVariables extends OperationVariables,
>(
result: ApolloQueryResult<TData>,
internalState: InternalState<TData, TVariables>,
observable: ObservableQuery<TData, TVariables>,
partialRefetch: boolean | undefined
): ApolloQueryResult<TData> {
// TODO: This code should be removed when the partialRefetch option is
Expand All @@ -645,9 +652,9 @@ function unsafeHandlePartialRefetch<
partialRefetch &&
!result.loading &&
(!result.data || Object.keys(result.data).length === 0) &&
internalState.observable.options.fetchPolicy !== "cache-only"
observable.options.fetchPolicy !== "cache-only"
) {
internalState.observable.refetch();
observable.refetch();
return {
...result,
loading: true,
Expand Down