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

feat(react-query): support react query v5 #434

Merged
merged 43 commits into from
Oct 25, 2023

Conversation

neil585456525
Copy link
Contributor

@neil585456525 neil585456525 commented Sep 28, 2023

Description

Let TypeScript React-Query plugin support V5 payload syntax.

Related #318

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as
    expected)
  • This change requires a documentation update

How Has This Been Tested?

  • Set reactQueryVersion to 5 in config.
  • Run my reproduce repo to check

Checklist:

  • I have followed the
    CONTRIBUTING doc and the
    style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules

@changeset-bot
Copy link

changeset-bot bot commented Sep 28, 2023

🦋 Changeset detected

Latest commit: 753e929

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

This PR includes changesets to release 1 package
Name Type
@graphql-codegen/typescript-react-query Major

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

@neil585456525 neil585456525 changed the title Feat/support react query v5 feat (react-query) support react query v5 Sep 28, 2023
@neil585456525 neil585456525 changed the title feat (react-query) support react query v5 feat(react-query): support react query v5 Sep 28, 2023
@neil585456525 neil585456525 marked this pull request as ready for review October 2, 2023 07:37
@rliljest
Copy link

rliljest commented Oct 2, 2023

There's one issue I'm noticing with custom options. It's currently generating code that looks like this:

export const useFooQuery = <
      TData = FooQuery,
      TError = Error
    >(
      variables: FooQueryVariables,
      options?: UseQueryOptions<FooQuery, TError, TData>
    ) =>
    useQuery<FooQuery, TError, TData>(
      {
    queryKey: ['Foo', variables],
    queryFn: gqlFetcher<FooQuery, FooQueryVariables>(FooDocument, variables),
    ...options
  }
    );

If you try to use it though, it throws a TS error:

useFooQuery({id: 1}, {enabled: false})
// Property 'queryKey' is missing in type '{ enabled: boolean; }' but required in type 'UseQueryOptions<FooQuery, Error, FooQuery, QueryKey>'

The issue is that the UseQueryOptions type now requires queryKey and queryFn, which are supplied by the generated code. If we could change the generated code to look like

  options?: Omit<UseQueryOptions<FooQuery, TError, TData>, 'queryKey' | 'queryFn'>

then it should work fine

@rliljest
Copy link

rliljest commented Oct 2, 2023

Wondering if it would make sense to include with this MR, or if it should be added to a future one: v5 adds support for suspense queries. Currently we have a flag for addInfiniteQuery which adds infinite queries in addition to the standard ones. We could add a new flag addSuspenseQuery which is false by default, but would also generate suspense queries.

So if both addInfiniteQuery and addSuspenseQuery were enabled, it would generate

  • useFooQuery
  • useInfiniteFooQuery
  • useSuspenseFooQuery
  • useSuspenseInfiniteFooQuery

@TkDodo
Copy link

TkDodo commented Oct 2, 2023

Yeah you would need those with v5 because the suspense:boolean flag was removed

@neil585456525
Copy link
Contributor Author

neil585456525 commented Oct 15, 2023

@rliljest Thx for cathing the bug, the update should fix the issue!

The updated code looks great and is able to get me a bit further. One issue I am noticing with the generated infinite query code: it looks like the third generic passed to useInfiniteQuery needs to be updated for v5. Currently TypeScript is only giving me a single page data response. In the generated infinite code for v5, we need to change

FYI, the first generic TData in the generated code like useInfiniteFooQuery means the expected return data value of the whole function, so it's better to directly change it to TData = InfiniteData<GetPersonQuery> instead of the solution you provided. But anyway, thx for ur time to do the research, it's indeed help a lots!

@rliljest
Copy link

rliljest commented Oct 17, 2023

@neil585456525 Tried this latest version, and it does work without errors when targeting react-query v5! Just noticed one issue that has not been brought up:

When using react-query v4, there is a breaking change in the generated infinite queries when addInfiniteQuery is used. With your changes, the first pageParamKey argument is removed. The value is actually unused in our generated code: I'm not sure if it was a holdout from react-query v3 support, or if it's only unused when you use a custom fetcher. Either way it appears to be a breaking change for v4 users, not sure if it was intended.

UPDATE: It looks like at least part of the issue is that I was on version 4 of @graphql-codegen/typescript-react-query, and the pageParamKey was removed in version 5

@neil585456525
Copy link
Contributor Author

neil585456525 commented Oct 17, 2023

@rliljest let me check the v4 compatibility, thx. It should be no break change for current usage.

@neil585456525
Copy link
Contributor Author

neil585456525 commented Oct 18, 2023

Hi @rliljest, after the research I found that the pageParamKey change which you have mentioned is intended. The newest changelog said that they have removed the unused pageParamKey parameter. But I found that the fetcher graph-request seems miss the modification. That's why you saw the break change but in the generated code, it doesn't use this parameter.
I think we can fix this issue also in this pr, since the change is target users who gonna use v5, but we still need to mark the break change in case some users want to upgrade the plugin but still using v4.

@neil585456525
Copy link
Contributor Author

Hi @saihaj , we have already done the discussion and modifications. Please review our pr to see if we miss anything for the release 🙏

@saihaj
Copy link
Collaborator

saihaj commented Oct 25, 2023

thank you so much everyone!

@saihaj saihaj merged commit 935b51f into dotansimha:main Oct 25, 2023
14 checks passed
@blaenk
Copy link

blaenk commented Nov 4, 2023

Happy to file an issue if this is indeed a problem, but I had been using preset: import-types all along so that types would be referred to as Types.MyType. It had been working fine, but it is broken now on @graphql-codegen/typescript-react-query 6.0.0 which I believe is largely implemented here, so maybe this inadvertently broke it? Not sure.

Either way thank you so much for this work to support the new react-query!

@lafest
Copy link

lafest commented Nov 9, 2023

FYI, useInfiniteQuery has more generics and you'd really need to set / respect them all, otherwise, some features might not work:

export function useInfiniteQuery<
  TQueryFnData,
  TError = DefaultError,
  TData = InfiniteData<TQueryFnData>,
  TQueryKey extends QueryKey = QueryKey,
  TPageParam = unknown,
>

without TQueryKey, you won't get the right type for queryKey when using it when its passed to the queryFn. Same for TPageParam - it's necessary to give you the correct type of pageParam in the queryFn:

useInfiniteQuery({
  queryKey: ['todos', { done: true }] as const,
  initialPageParam: 1,
  getNextPageParam: () => undefined,
  queryFn: ({ queryKey, pageParam }) => 
//            ⬆️ queryKey is readonly['todos', readonly { done: true }]
//                       ⬆️ pageParam is number
})

@TkDodo @neil585456525 is this promblem resolved?

after updateing @graphql-codegen/typescript-react-query@6.0.0, I met issue with pageParam type like below.
스크린샷 2023-11-09 11 54 20

is there any solution for this?

my codegen config is like below.

import type { CodegenConfig } from '@graphql-codegen/cli';

const config: CodegenConfig = {
  documents: 'src/graphql/query/**/*.graphql',
  generates: {
    'src/graphql/generated/index.ts': {
      config: {
        addInfiniteQuery: true,
        avoidOptionals: true,
        exposeQueryKeys: true,
        fetcher: 'apis/GraphQLApi#fetcher',
        legacyMode: false,
        reactQueryVersion: 5,
        skipTypename: true,
      },
      plugins: ['typescript', 'typescript-operations', 'typescript-react-query'],
    },
  },
  hooks: {
    afterOneFileWrite: ['prettier --write'],
  },
  overwrite: true,
  schema: 'src/graphql/schema.ts',
};

export default config;

@dohomi
Copy link

dohomi commented Nov 9, 2023

Happy to file an issue if this is indeed a problem, but I had been using preset: import-types all along so that types would be referred to as Types.MyType. It had been working fine, but it is broken now on @graphql-codegen/typescript-react-query 6.0.0 which I believe is largely implemented here, so maybe this inadvertently broke it? Not sure.

Either way thank you so much for this work to support the new react-query!

I have the exact same issue, v6 seems to break the preset import-types, it would be great if there is a solution for this.

@groomain
Copy link

groomain commented Dec 4, 2023

#434 (comment)

Hi,

I also have an issue with infinite query. I tried upgrading to the last version.

I think this type is wrong

export function useInfiniteQuery<
  TQueryFnData,
  TError = DefaultError,
  TData = InfiniteData<TQueryFnData>,
  TQueryKey extends QueryKey = QueryKey,
  TPageParam = unknown,
>

Is it the good type for TData @TkDodo @neil585456525 ?

The TData = InfiniteData<TQueryFnData>, should it be only TQueryFnData ?

because with the version 6.0.0 all my infinite queries types in typescript does not match what is really returned.

I have a strange typescript error on what I should have on my result data. I should have an array

{
  pageParams : ... [],
  pages:  {
    pageParams : unknown[],
    pages: result[]
    }[]
  }

but what I really have is not an array

{
    pageParams : ...,
    pages: result[]
}

This type is used for the result and it already use InfiniteData. So I thinks it should not be in the useInfiniteQuery type params

export interface InfiniteQueryObserverSuccessResult<
  TData = unknown,
  TError = unknown,
> extends InfiniteQueryObserverBaseResult<TData, TError> {
  data: InfiniteData<TData>
  error: null
  isError: false
  isLoading: false
  isLoadingError: false
  isRefetchError: false
  isSuccess: true
  status: 'success'
}

Thanks for your help!

@groomain
Copy link

groomain commented Dec 5, 2023

@neil585456525 do you want that I create a new issue?

@neil585456525
Copy link
Contributor Author

@groomain It will be great thx!
let me schedule a time to investigate the above issues.

@kieranm
Copy link

kieranm commented Dec 9, 2023

We've had to downgrade back because our outputted code is missing the Types. prefix on all types and won't compile.

Thank you for your hard work on this and let us know if you need any more info to help investigate!

@groomain
Copy link

groomain commented Dec 17, 2023

Done @neil585456525

#541

@anthonyn60
Copy link

FYI, useInfiniteQuery has more generics and you'd really need to set / respect them all, otherwise, some features might not work:

export function useInfiniteQuery<
  TQueryFnData,
  TError = DefaultError,
  TData = InfiniteData<TQueryFnData>,
  TQueryKey extends QueryKey = QueryKey,
  TPageParam = unknown,
>

without TQueryKey, you won't get the right type for queryKey when using it when its passed to the queryFn. Same for TPageParam - it's necessary to give you the correct type of pageParam in the queryFn:

useInfiniteQuery({
  queryKey: ['todos', { done: true }] as const,
  initialPageParam: 1,
  getNextPageParam: () => undefined,
  queryFn: ({ queryKey, pageParam }) => 
//            ⬆️ queryKey is readonly['todos', readonly { done: true }]
//                       ⬆️ pageParam is number
})

@TkDodo @neil585456525 is this promblem resolved?

after updateing @graphql-codegen/typescript-react-query@6.0.0, I met issue with pageParam type like below. 스크린샷 2023-11-09 11 54 20

is there any solution for this?

my codegen config is like below.

import type { CodegenConfig } from '@graphql-codegen/cli';

const config: CodegenConfig = {
  documents: 'src/graphql/query/**/*.graphql',
  generates: {
    'src/graphql/generated/index.ts': {
      config: {
        addInfiniteQuery: true,
        avoidOptionals: true,
        exposeQueryKeys: true,
        fetcher: 'apis/GraphQLApi#fetcher',
        legacyMode: false,
        reactQueryVersion: 5,
        skipTypename: true,
      },
      plugins: ['typescript', 'typescript-operations', 'typescript-react-query'],
    },
  },
  hooks: {
    afterOneFileWrite: ['prettier --write'],
  },
  overwrite: true,
  schema: 'src/graphql/schema.ts',
};

export default config;

@lafest did you find a solution to your issue? I had the same thing pop up. Opened an issue here #583

@South-Paw
Copy link

It would be great if someone could update the docs to mention that this does plugin support RQ5 🚀

Took me more than a little while to discover this PR and the new option being added to get it working.

@saihaj
Copy link
Collaborator

saihaj commented Feb 19, 2024

It would be great if someone could update the docs to mention that this does plugin support RQ5 🚀

Took me more than a little while to discover this PR and the new option being added to get it working.

hey @South-Paw do you want to send in a PR?

@South-Paw
Copy link

I did make a start on writing some new docs - but quickly realized there was a fair bit more that needed changing/updating than simply stating it supports RQ5. A fair bit of that page is out of sync with the package IMO.

Unfortunately I think someone who's a bit more familiar with the package would be best placed to start on it too. I'm not well placed at this time to take on that task right now (time and familiarity wise).

draftcode added a commit to draftcode/graphql-code-generator-community that referenced this pull request Aug 7, 2024
…ableQuery

Implement `useBackgroundQuery` and `useLoadableQuery` hooks for React
Apollo. These APIs are added in 3.8.0 and 3.9.0 respectively.

Suspense support in Apollo Client provides 5 new hooks as described in
https://www.apollographql.com/docs/react/data/suspense/.
`useSuspenseQuery` support was added in
dotansimha#434.
Among other 4 hooks, `useBackgroundQuery` and `useLoadableQuery` are
the ones that take a GraphQL document as an argument like other existing
hooks. The support for these hooks are added in this change.

Other hooks, `useQueryRefHanders` and `useReadQuery` do not take a
GraphQL document as an argument, so they do not need to be added as the
generated code. Both take the return values of other hooks as an
argument, and the types are inferred from it.
draftcode added a commit to draftcode/graphql-code-generator-community that referenced this pull request Aug 7, 2024
…ableQuery

Implement `useBackgroundQuery` and `useLoadableQuery` hooks for React
Apollo. These APIs are added in 3.8.0 and 3.9.0 respectively.

Suspense support in Apollo Client provides 5 new hooks as described in
https://www.apollographql.com/docs/react/data/suspense/.
`useSuspenseQuery` support was added in
dotansimha#434.
Among other 4 hooks, `useBackgroundQuery` and `useLoadableQuery` are
the ones that take a GraphQL document as an argument like other existing
hooks. The support for these hooks are added in this change.

Other hooks, `useQueryRefHanders` and `useReadQuery` do not take a
GraphQL document as an argument, so they do not need to be added as the
generated code. Both take the return values of other hooks as an
argument, and the types are inferred from it.

Follow up to
dotansimha#388.
Fixes dotansimha#477.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.