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

Improve useBackgroundQuery type interface and add type tests #10951

Merged
merged 7 commits into from
Jun 9, 2023

Conversation

alessbell
Copy link
Contributor

Closes #10878. Improves types for useBackgroundQuery and adds type tests.

Follow-on work to support returning partial data (including representing as DeepPartial<TData> in the response type) will be tackled as part of #10893.

Checklist:

  • If this PR contains changes to the library itself (not necessary for e.g. docs updates), please include a changeset (see CONTRIBUTING.md)
  • If this PR is a new feature, please reference an issue where a consensus about the design was reached (not necessary for small changes)
  • Make sure all of the significant new logic is covered by tests

@changeset-bot
Copy link

changeset-bot bot commented Jun 5, 2023

🦋 Changeset detected

Latest commit: dcc6933

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@apollo/client Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@github-actions
Copy link
Contributor

github-actions bot commented Jun 5, 2023

size-limit report 📦

Path Size
dist/apollo-client.min.cjs 36.55 KB (0%)
import { ApolloClient, InMemoryCache, HttpLink } from "dist/main.cjs" 42.95 KB (0%)
import { ApolloClient, InMemoryCache, HttpLink } from "dist/index.js" 32.44 KB (0%)
import { ApolloProvider } from "dist/react/index.js" 1.3 KB (0%)
import { useQuery } from "dist/react/index.js" 4.34 KB (0%)
import { useLazyQuery } from "dist/react/index.js" 4.66 KB (0%)
import { useMutation } from "dist/react/index.js" 2.57 KB (0%)
import { useSubscription } from "dist/react/index.js" 2.34 KB (0%)
import { useSuspenseQuery } from "dist/react/index.js" 3.45 KB (0%)
import { useBackgroundQuery } from "dist/react/index.js" 2.89 KB (0%)
import { useReadQuery } from "dist/react/index.js" 1.65 KB (0%)
import { useFragment } from "dist/react/index.js" 2.12 KB (0%)

@@ -131,6 +135,37 @@ function renderIntegrationTest({
return { ...rest, query, client: _client, renders };
}

interface VariablesCaseData {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Lifted VariablesCaseData/VariablesCaseVariables up to the file scope to make type tests easier to write, a nice pattern borrowed from the useSuspenseQuery tests

refetch: (
variables?: Partial<OperationVariables> | undefined
) => Promise<ApolloQueryResult<Data>>;
refetch: RefetchFunction<Data, OperationVariables>;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

refetch type is much cleaner now ✨

expectTypeOf(explicit).not.toEqualTypeOf<VariablesCaseData | undefined>();
});

// TODO: https://github.com/apollographql/apollo-client/issues/10893
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The remaining five type tests from useSuspenseQuery have references to returnPartialData and will be completed as part of the work in #10893

@@ -61,8 +61,6 @@ export type SubscribeToMoreFunction<
TVariables extends OperationVariables
> = ObservableQueryFields<TData, TVariables>['subscribeToMore'];

export type Version = 'main' | 'network';
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Pretty sure this is the last vestige of all your refactoring work, @jerelmiller - thought I'd zap it ⚡

Copy link
Member

Choose a reason for hiding this comment

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

Good catch! Thanks for removing this!

@@ -6928,17 +6928,17 @@ describe('useSuspenseQuery', () => {
it('returns TData | undefined with errorPolicy: "all"', () => {
const { query } = useVariablesQueryCase();

const { data: inferred } = useSuspenseQuery<
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Found some tests that seemed to have the inferred/explicit labelling flipped and figured I'd include those small fixes in this PR, let me know if these look good to you

Copy link
Member

Choose a reason for hiding this comment

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

Thank you! I noticed that too when adding skip support. Appreciate it!

import { invariant } from '../../utilities/globals';

export type UseBackgroundQueryResult<
TData = any,
TData = unknown,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

any unknown 🎉

}
];

export function useBackgroundQuery<
TData = any,
TData,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These overloads are taken from useSuspenseQuery with the caveat that returnPartialData/refetchWritePolicy are not available in useBackgroundQuery yet (to be added in #10893)

@@ -20,6 +20,10 @@ import type {
} from '../../core';
import type { SuspenseCache } from '../cache';

/* QueryReference type */

export type { QueryReference } from '../cache/QueryReference';
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This should fix the current import { QueryReference } from "@apollo/client/react/cache/QueryReference"; situation

Copy link
Member

Choose a reason for hiding this comment

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

Ah thank you!

Copy link
Member

@jerelmiller jerelmiller left a comment

Choose a reason for hiding this comment

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

Really great to get this dialed in! Thanks for putting this together!

SuspenseQueryHookOptions<NoInfer<TData>, NoInfer<TVariables>>,
'returnPartialData' | 'refetchWritePolicy'
> & {
returnPartialData: true;
Copy link
Member

Choose a reason for hiding this comment

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

Since this option is being added in a separate PR and because you have Omit<..., 'returnPartialData'> in this overload, do we need this for this PR? I think you can omit this case for now and add it in #10960

@@ -61,8 +61,6 @@ export type SubscribeToMoreFunction<
TVariables extends OperationVariables
> = ObservableQueryFields<TData, TVariables>['subscribeToMore'];

export type Version = 'main' | 'network';
Copy link
Member

Choose a reason for hiding this comment

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

Good catch! Thanks for removing this!

@@ -20,6 +20,10 @@ import type {
} from '../../core';
import type { SuspenseCache } from '../cache';

/* QueryReference type */

export type { QueryReference } from '../cache/QueryReference';
Copy link
Member

Choose a reason for hiding this comment

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

Ah thank you!

@alessbell alessbell merged commit 2e833b2 into release-3.8 Jun 9, 2023
@alessbell alessbell deleted the issue-10878-improve-useBackgroundQuery-types branch June 9, 2023 17:26
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 10, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants