From 4a6e86aeaf6685abf0dd23110784848c8b085735 Mon Sep 17 00:00:00 2001 From: Lenz Weber-Tronic Date: Mon, 15 Jul 2024 10:27:49 +0200 Subject: [PATCH] optimize `useQuery` result handling (#11954) * remove unused code path Coverage shows that this code path was never hit - in the case of a `standby` fetchPolicy, `resultData.current` would already have been reset in `useResubscribeIfNecessary` * move code from `toQueryResult` to `setResult` While there are three code paths to `toQueryResult`, the other two are guaranteed to never have an `errors` property * add undocumented `errors` field to types and deprecate it * remove `useHandleSkip` instead, we calculate and memoize an "override result" in `useObservableSubscriptionResult` * remove now-obsolete `originalResult` symbol property * fix typo, chores * adjustment for compiler compat * review feedback * changeset * api-report --- .api-reports/api-report-core.api.md | 3 +- .api-reports/api-report-react.api.md | 5 +- .../api-report-react_components.api.md | 5 +- .api-reports/api-report-react_context.api.md | 5 +- .api-reports/api-report-react_hoc.api.md | 3 +- .api-reports/api-report-react_hooks.api.md | 5 +- .api-reports/api-report-react_internal.api.md | 5 +- .api-reports/api-report-react_ssr.api.md | 5 +- .api-reports/api-report-testing.api.md | 3 +- .api-reports/api-report-testing_core.api.md | 3 +- .api-reports/api-report-utilities.api.md | 3 +- .api-reports/api-report.api.md | 5 +- .changeset/proud-humans-begin.md | 5 + .size-limits.json | 2 +- src/react/hooks/useQuery.ts | 154 +++++++----------- src/react/types/types.ts | 7 +- 16 files changed, 95 insertions(+), 123 deletions(-) create mode 100644 .changeset/proud-humans-begin.md diff --git a/.api-reports/api-report-core.api.md b/.api-reports/api-report-core.api.md index e91bf2384b9..77b976d6673 100644 --- a/.api-reports/api-report-core.api.md +++ b/.api-reports/api-report-core.api.md @@ -2303,8 +2303,7 @@ interface WriteContext extends ReadMergeModifyContext { // src/core/QueryManager.ts:124:5 - (ae-forgotten-export) The symbol "MutationStoreValue" needs to be exported by the entry point index.d.ts // src/core/QueryManager.ts:158:5 - (ae-forgotten-export) The symbol "LocalState" needs to be exported by the entry point index.d.ts // src/core/QueryManager.ts:391:7 - (ae-forgotten-export) The symbol "UpdateQueries" needs to be exported by the entry point index.d.ts -// src/core/watchQueryOptions.ts:272:2 - (ae-forgotten-export) The symbol "IgnoreModifier" needs to be exported by the entry point index.d.ts -// src/core/watchQueryOptions.ts:272:2 - (ae-forgotten-export) The symbol "UpdateQueryFn" needs to be exported by the entry point index.d.ts +// src/core/watchQueryOptions.ts:275:2 - (ae-forgotten-export) The symbol "UpdateQueryFn" needs to be exported by the entry point index.d.ts // src/link/http/selectHttpOptionsAndBody.ts:128:32 - (ae-forgotten-export) The symbol "HttpQueryOptions" needs to be exported by the entry point index.d.ts // (No @packageDocumentation comment for this package) diff --git a/.api-reports/api-report-react.api.md b/.api-reports/api-report-react.api.md index f2acc1ef23f..dc70efc3151 100644 --- a/.api-reports/api-report-react.api.md +++ b/.api-reports/api-report-react.api.md @@ -1695,6 +1695,8 @@ export interface QueryResult; data: TData | undefined; error?: ApolloError; + // @deprecated (undocumented) + errors?: ReadonlyArray; loading: boolean; networkStatus: NetworkStatus; observable: ObservableQuery; @@ -2328,8 +2330,7 @@ interface WatchQueryOptions; data: TData | undefined; error?: ApolloError; + // @deprecated (undocumented) + errors?: ReadonlyArray; loading: boolean; networkStatus: NetworkStatus; observable: ObservableQuery; @@ -1807,8 +1809,7 @@ interface WatchQueryOptions; data: TData | undefined; error?: ApolloError; + // @deprecated (undocumented) + errors?: ReadonlyArray; loading: boolean; networkStatus: NetworkStatus; observable: ObservableQuery; @@ -1727,8 +1729,7 @@ interface WatchQueryOptions; data: TData | undefined; error?: ApolloError; + // @deprecated (undocumented) + errors?: ReadonlyArray; loading: boolean; networkStatus: NetworkStatus; observable: ObservableQuery; @@ -2152,8 +2154,7 @@ interface WatchQueryOptions; data: TData | undefined; error?: ApolloError; + // @deprecated (undocumented) + errors?: ReadonlyArray; loading: boolean; networkStatus: NetworkStatus; observable: ObservableQuery; @@ -2215,8 +2217,7 @@ export function wrapQueryRef(inter // src/core/QueryManager.ts:391:7 - (ae-forgotten-export) The symbol "UpdateQueries" needs to be exported by the entry point index.d.ts // src/core/types.ts:174:3 - (ae-forgotten-export) The symbol "MutationQueryReducer" needs to be exported by the entry point index.d.ts // src/core/types.ts:203:5 - (ae-forgotten-export) The symbol "Resolver" needs to be exported by the entry point index.d.ts -// src/core/watchQueryOptions.ts:272:2 - (ae-forgotten-export) The symbol "IgnoreModifier" needs to be exported by the entry point index.d.ts -// src/core/watchQueryOptions.ts:272:2 - (ae-forgotten-export) The symbol "UpdateQueryFn" needs to be exported by the entry point index.d.ts +// src/core/watchQueryOptions.ts:275:2 - (ae-forgotten-export) The symbol "UpdateQueryFn" needs to be exported by the entry point index.d.ts // src/react/hooks/useBackgroundQuery.ts:38:3 - (ae-forgotten-export) The symbol "SubscribeToMoreFunction" needs to be exported by the entry point index.d.ts // src/react/hooks/useBackgroundQuery.ts:54:3 - (ae-forgotten-export) The symbol "FetchMoreFunction" needs to be exported by the entry point index.d.ts // src/react/hooks/useBackgroundQuery.ts:78:4 - (ae-forgotten-export) The symbol "RefetchFunction" needs to be exported by the entry point index.d.ts diff --git a/.api-reports/api-report-react_ssr.api.md b/.api-reports/api-report-react_ssr.api.md index 1816993de87..4dc802fa4c6 100644 --- a/.api-reports/api-report-react_ssr.api.md +++ b/.api-reports/api-report-react_ssr.api.md @@ -1412,6 +1412,8 @@ interface QueryResult; data: TData | undefined; error?: ApolloError; + // @deprecated (undocumented) + errors?: ReadonlyArray; loading: boolean; networkStatus: NetworkStatus; observable: ObservableQuery; @@ -1712,8 +1714,7 @@ interface WatchQueryOptions(it: (...args: TArgs // src/core/QueryManager.ts:391:7 - (ae-forgotten-export) The symbol "UpdateQueries" needs to be exported by the entry point index.d.ts // src/core/types.ts:174:3 - (ae-forgotten-export) The symbol "MutationQueryReducer" needs to be exported by the entry point index.d.ts // src/core/types.ts:203:5 - (ae-forgotten-export) The symbol "Resolver" needs to be exported by the entry point index.d.ts -// src/core/watchQueryOptions.ts:272:2 - (ae-forgotten-export) The symbol "IgnoreModifier" needs to be exported by the entry point index.d.ts -// src/core/watchQueryOptions.ts:272:2 - (ae-forgotten-export) The symbol "UpdateQueryFn" needs to be exported by the entry point index.d.ts +// src/core/watchQueryOptions.ts:275:2 - (ae-forgotten-export) The symbol "UpdateQueryFn" needs to be exported by the entry point index.d.ts // (No @packageDocumentation comment for this package) diff --git a/.api-reports/api-report-testing_core.api.md b/.api-reports/api-report-testing_core.api.md index 07be1d04650..1160c018bfe 100644 --- a/.api-reports/api-report-testing_core.api.md +++ b/.api-reports/api-report-testing_core.api.md @@ -1739,8 +1739,7 @@ export function withWarningSpy(it: (...args: TArgs // src/core/QueryManager.ts:391:7 - (ae-forgotten-export) The symbol "UpdateQueries" needs to be exported by the entry point index.d.ts // src/core/types.ts:174:3 - (ae-forgotten-export) The symbol "MutationQueryReducer" needs to be exported by the entry point index.d.ts // src/core/types.ts:203:5 - (ae-forgotten-export) The symbol "Resolver" needs to be exported by the entry point index.d.ts -// src/core/watchQueryOptions.ts:272:2 - (ae-forgotten-export) The symbol "IgnoreModifier" needs to be exported by the entry point index.d.ts -// src/core/watchQueryOptions.ts:272:2 - (ae-forgotten-export) The symbol "UpdateQueryFn" needs to be exported by the entry point index.d.ts +// src/core/watchQueryOptions.ts:275:2 - (ae-forgotten-export) The symbol "UpdateQueryFn" needs to be exported by the entry point index.d.ts // (No @packageDocumentation comment for this package) diff --git a/.api-reports/api-report-utilities.api.md b/.api-reports/api-report-utilities.api.md index 2d2108cade6..8754c6c0da2 100644 --- a/.api-reports/api-report-utilities.api.md +++ b/.api-reports/api-report-utilities.api.md @@ -2670,8 +2670,7 @@ interface WriteContext extends ReadMergeModifyContext { // src/core/QueryManager.ts:391:7 - (ae-forgotten-export) The symbol "UpdateQueries" needs to be exported by the entry point index.d.ts // src/core/types.ts:174:3 - (ae-forgotten-export) The symbol "MutationQueryReducer" needs to be exported by the entry point index.d.ts // src/core/types.ts:203:5 - (ae-forgotten-export) The symbol "Resolver" needs to be exported by the entry point index.d.ts -// src/core/watchQueryOptions.ts:272:2 - (ae-forgotten-export) The symbol "IgnoreModifier" needs to be exported by the entry point index.d.ts -// src/core/watchQueryOptions.ts:272:2 - (ae-forgotten-export) The symbol "UpdateQueryFn" needs to be exported by the entry point index.d.ts +// src/core/watchQueryOptions.ts:275:2 - (ae-forgotten-export) The symbol "UpdateQueryFn" needs to be exported by the entry point index.d.ts // src/utilities/graphql/storeUtils.ts:226:12 - (ae-forgotten-export) The symbol "storeKeyNameStringify" needs to be exported by the entry point index.d.ts // src/utilities/policies/pagination.ts:76:3 - (ae-forgotten-export) The symbol "TRelayEdge" needs to be exported by the entry point index.d.ts // src/utilities/policies/pagination.ts:77:3 - (ae-forgotten-export) The symbol "TRelayPageInfo" needs to be exported by the entry point index.d.ts diff --git a/.api-reports/api-report.api.md b/.api-reports/api-report.api.md index ca7d586070f..7fdbd19f329 100644 --- a/.api-reports/api-report.api.md +++ b/.api-reports/api-report.api.md @@ -2266,6 +2266,8 @@ export interface QueryResult; data: TData | undefined; error?: ApolloError; + // @deprecated (undocumented) + errors?: ReadonlyArray; loading: boolean; networkStatus: NetworkStatus; observable: ObservableQuery; @@ -3016,8 +3018,7 @@ interface WriteContext extends ReadMergeModifyContext { // src/core/QueryManager.ts:124:5 - (ae-forgotten-export) The symbol "MutationStoreValue" needs to be exported by the entry point index.d.ts // src/core/QueryManager.ts:158:5 - (ae-forgotten-export) The symbol "LocalState" needs to be exported by the entry point index.d.ts // src/core/QueryManager.ts:391:7 - (ae-forgotten-export) The symbol "UpdateQueries" needs to be exported by the entry point index.d.ts -// src/core/watchQueryOptions.ts:272:2 - (ae-forgotten-export) The symbol "IgnoreModifier" needs to be exported by the entry point index.d.ts -// src/core/watchQueryOptions.ts:272:2 - (ae-forgotten-export) The symbol "UpdateQueryFn" needs to be exported by the entry point index.d.ts +// src/core/watchQueryOptions.ts:275:2 - (ae-forgotten-export) The symbol "UpdateQueryFn" needs to be exported by the entry point index.d.ts // src/link/http/selectHttpOptionsAndBody.ts:128:32 - (ae-forgotten-export) The symbol "HttpQueryOptions" needs to be exported by the entry point index.d.ts // src/react/hooks/useBackgroundQuery.ts:38:3 - (ae-forgotten-export) The symbol "SubscribeToMoreFunction" needs to be exported by the entry point index.d.ts // src/react/hooks/useBackgroundQuery.ts:54:3 - (ae-forgotten-export) The symbol "FetchMoreFunction" needs to be exported by the entry point index.d.ts diff --git a/.changeset/proud-humans-begin.md b/.changeset/proud-humans-begin.md new file mode 100644 index 00000000000..059cffa65fc --- /dev/null +++ b/.changeset/proud-humans-begin.md @@ -0,0 +1,5 @@ +--- +"@apollo/client": patch +--- + +Document (and deprecate) the previously undocumented `errors` property on the `useQuery` `QueryResult` type. diff --git a/.size-limits.json b/.size-limits.json index c6da4a36f8f..60d013b1b4f 100644 --- a/.size-limits.json +++ b/.size-limits.json @@ -1,4 +1,4 @@ { - "dist/apollo-client.min.cjs": 40206, + "dist/apollo-client.min.cjs": 40162, "import { ApolloClient, InMemoryCache, HttpLink } from \"dist/index.js\" (production)": 32999 } diff --git a/src/react/hooks/useQuery.ts b/src/react/hooks/useQuery.ts index 8f56c095fe5..5a483f9b4e8 100644 --- a/src/react/hooks/useQuery.ts +++ b/src/react/hooks/useQuery.ts @@ -61,14 +61,10 @@ const { prototype: { hasOwnProperty }, } = Object; -const originalResult = Symbol(); -interface InternalQueryResult - extends Omit< - QueryResult, - Exclude, "variables"> - > { - [originalResult]: ApolloQueryResult; -} +type InternalQueryResult = Omit< + QueryResult, + Exclude, "variables"> +>; function noop() {} export const lastWatchOptions = Symbol(); @@ -295,22 +291,14 @@ export function useQueryInternals< Omit, "variables"> >(() => bindObservableMethods(observable), [observable]); - useHandleSkip( - resultData, // might get mutated during render - observable, - client, - options, - watchQueryOptions, - disableNetworkFetches, - isSyncSSR - ); - useRegisterSSRObservable(observable, renderPromises, ssrAllowed); const result = useObservableSubscriptionResult( resultData, observable, client, + options, + watchQueryOptions, disableNetworkFetches, partialRefetch, isSyncSSR, @@ -337,9 +325,11 @@ function useObservableSubscriptionResult< resultData: InternalResult, observable: ObservableQuery, client: ApolloClient, + options: QueryHookOptions, NoInfer>, + watchQueryOptions: Readonly>, disableNetworkFetches: boolean, partialRefetch: boolean | undefined, - skipSubscribing: boolean, + isSyncSSR: boolean, callbacks: { onCompleted: (data: TData) => void; onError: (error: ApolloError) => void; @@ -356,6 +346,37 @@ function useObservableSubscriptionResult< callbackRef.current = callbacks; }); + const resultOverride = + ( + (isSyncSSR || disableNetworkFetches) && + options.ssr === false && + !options.skip + ) ? + // If SSR has been explicitly disabled, and this function has been called + // on the server side, return the default loading state. + ssrDisabledResult + : options.skip || watchQueryOptions.fetchPolicy === "standby" ? + // When skipping a query (ie. we're not querying for data but still want to + // render children), make sure the `data` is cleared out and `loading` is + // set to `false` (since we aren't loading anything). + // + // NOTE: We no longer think this is the correct behavior. Skipping should + // not automatically set `data` to `undefined`, but instead leave the + // previous data in place. In other words, skipping should not mandate that + // previously received data is all of a sudden removed. Unfortunately, + // changing this is breaking, so we'll have to wait until Apollo Client 4.0 + // to address this. + skipStandbyResult + : void 0; + + const previousData = resultData.previousData; + const currentResultOverride = React.useMemo( + () => + resultOverride && + toQueryResult(resultOverride, previousData, observable, client), + [client, observable, resultOverride, previousData] + ); + return useSyncExternalStore( React.useCallback( (handleStoreChange) => { @@ -363,7 +384,7 @@ function useObservableSubscriptionResult< // keep it as a dependency of this effect, even though it's not used disableNetworkFetches; - if (skipSubscribing) { + if (isSyncSSR) { return () => {}; } @@ -447,7 +468,7 @@ function useObservableSubscriptionResult< [ disableNetworkFetches, - skipSubscribing, + isSyncSSR, observable, resultData, partialRefetch, @@ -455,6 +476,7 @@ function useObservableSubscriptionResult< ] ), () => + currentResultOverride || getCurrentResult( resultData, observable, @@ -463,6 +485,7 @@ function useObservableSubscriptionResult< client ), () => + currentResultOverride || getCurrentResult( resultData, observable, @@ -488,60 +511,6 @@ function useRegisterSSRObservable( } } -function useHandleSkip< - TData = any, - TVariables extends OperationVariables = OperationVariables, ->( - /** this hook will mutate properties on `resultData` */ - resultData: InternalResult, - observable: ObsQueryWithMeta, - client: ApolloClient, - options: QueryHookOptions, NoInfer>, - watchQueryOptions: Readonly>, - disableNetworkFetches: boolean, - isSyncSSR: boolean -) { - if ( - (isSyncSSR || disableNetworkFetches) && - options.ssr === false && - !options.skip - ) { - // If SSR has been explicitly disabled, and this function has been called - // on the server side, return the default loading state. - resultData.current = toQueryResult( - ssrDisabledResult, - resultData.previousData, - observable, - client - ); - } else if (options.skip || watchQueryOptions.fetchPolicy === "standby") { - // When skipping a query (ie. we're not querying for data but still want to - // render children), make sure the `data` is cleared out and `loading` is - // set to `false` (since we aren't loading anything). - // - // NOTE: We no longer think this is the correct behavior. Skipping should - // not automatically set `data` to `undefined`, but instead leave the - // previous data in place. In other words, skipping should not mandate that - // previously received data is all of a sudden removed. Unfortunately, - // changing this is breaking, so we'll have to wait until Apollo Client 4.0 - // to address this. - resultData.current = toQueryResult( - skipStandbyResult, - resultData.previousData, - observable, - client - ); - } else if ( - // reset result if the last render was skipping for some reason, - // but this render isn't skipping anymore - resultData.current && - (resultData.current[originalResult] === ssrDisabledResult || - resultData.current[originalResult] === skipStandbyResult) - ) { - resultData.current = void 0; - } -} - // this hook is not compatible with any rules of React, and there's no good way to rewrite it. // it should stay a separate hook that will not be optimized by the compiler function useResubscribeIfNecessary< @@ -693,6 +662,15 @@ function setResult( if (previousResult && previousResult.data) { resultData.previousData = previousResult.data; } + + if (!nextResult.error && isNonEmptyArray(nextResult.errors)) { + // Until a set naming convention for networkError and graphQLErrors is + // decided upon, we map errors (graphQLErrors) to the error options. + // TODO: Is it possible for both result.error and result.errors to be + // defined here? + nextResult.error = new ApolloError({ graphQLErrors: nextResult.errors }); + } + resultData.current = toQueryResult( unsafeHandlePartialRefetch(nextResult, observable, partialRefetch), resultData.previousData, @@ -702,16 +680,12 @@ function setResult( // Calling state.setResult always triggers an update, though some call sites // perform additional equality checks before committing to an update. forceUpdate(); - handleErrorOrCompleted( - nextResult, - previousResult?.[originalResult], - callbacks - ); + handleErrorOrCompleted(nextResult, previousResult?.networkStatus, callbacks); } function handleErrorOrCompleted( result: ApolloQueryResult, - previousResult: ApolloQueryResult | undefined, + previousNetworkStatus: NetworkStatus | undefined, callbacks: Callbacks ) { if (!result.loading) { @@ -724,7 +698,7 @@ function handleErrorOrCompleted( callbacks.onError(error); } else if ( result.data && - previousResult?.networkStatus !== result.networkStatus && + previousNetworkStatus !== result.networkStatus && result.networkStatus === NetworkStatus.ready ) { callbacks.onCompleted(result.data); @@ -799,21 +773,7 @@ export function toQueryResult( variables: observable.variables, called: result !== ssrDisabledResult && result !== skipStandbyResult, previousData, - } satisfies Omit< - InternalQueryResult, - typeof originalResult - > as InternalQueryResult; - // non-enumerable property to hold the original result, for referential equality checks - Object.defineProperty(queryResult, originalResult, { value: result }); - - if (!queryResult.error && isNonEmptyArray(result.errors)) { - // Until a set naming convention for networkError and graphQLErrors is - // decided upon, we map errors (graphQLErrors) to the error options. - // TODO: Is it possible for both result.error and result.errors to be - // defined here? - queryResult.error = new ApolloError({ graphQLErrors: result.errors }); - } - + }; return queryResult; } diff --git a/src/react/types/types.ts b/src/react/types/types.ts index cd2e6df1ccb..ab4444596f4 100644 --- a/src/react/types/types.ts +++ b/src/react/types/types.ts @@ -1,5 +1,5 @@ import type * as ReactTypes from "react"; -import type { DocumentNode } from "graphql"; +import type { DocumentNode, GraphQLFormattedError } from "graphql"; import type { TypedDocumentNode } from "@graphql-typed-document-node/core"; import type { @@ -148,6 +148,11 @@ export interface QueryResult< previousData?: TData; /** {@inheritDoc @apollo/client!QueryResultDocumentation#error:member} */ error?: ApolloError; + /** + * @deprecated This property will be removed in a future version of Apollo Client. + * Please use `error.graphQLErrors` instead. + */ + errors?: ReadonlyArray; /** {@inheritDoc @apollo/client!QueryResultDocumentation#loading:member} */ loading: boolean; /** {@inheritDoc @apollo/client!QueryResultDocumentation#networkStatus:member} */