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

Hotfix for typescript react query infinite query breaking change #8567

Conversation

EandrewJones
Copy link
Contributor

Description

As per @marcmll's comment #8497 (comment), the change introduced in PR #8497 breaks infinite query calls for some users if pageParam is undefined which happen on the first call.

Related #8566

The code now safeguards against this via a ternary operator that check whether pageParam is defined and only overwrites variable value when it's defined.

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

  • Updated the typescript-react-query plugin test suite (packages/plugins/typescript/react-query/tests/react-query.spec.ts) to match the new outputs

Test Environment:

  • OS: Ubuntu 18.04
  • NodeJS: v14.20.1

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 Nov 2, 2022

🦋 Changeset detected

Latest commit: cc7f529

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 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

Copy link

@marcmll marcmll left a comment

Choose a reason for hiding this comment

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

Thank you so much for your quick response and for opening a hotfix so quickly!

@EandrewJones
Copy link
Contributor Author

EandrewJones commented Nov 3, 2022 via email

@wileymc
Copy link

wileymc commented Nov 3, 2022

this is great work! lets get this merged :)

@saihaj
Copy link
Collaborator

saihaj commented Nov 4, 2022

@wileymc
Copy link

wileymc commented Nov 5, 2022

Interesting, after updating I'm still getting tsc complaining about:

Argument of type '{ [x: string]: any; }' is not assignable to parameter of type 'Exact<{ [key: string]: never; }>'.
  'string' index signatures are incompatible.
    Type 'any' is not assignable to type 'never'.

for infinite queries. any ideas?

@EandrewJones
Copy link
Contributor Author

EandrewJones commented Nov 5, 2022 via email

@wileymc
Copy link

wileymc commented Nov 5, 2022

Hi @EandrewJones. Here is one example of this in a generated query: https://github.com/populist-vote/web/blob/w/upgrade-rq/generated.ts#L3676-L3679

Our codegen.yml: https://github.com/populist-vote/web/blob/w/upgrade-rq/codegen.yml

And the associated .gql: https://github.com/populist-vote/web/blob/w/upgrade-rq/graphql/mutations/auth.graphql#L13-L33

Note, the offending generated hooks are for queries that we'll never need an infinite query for so perhaps there is a way to simply ignore these?

I can get to get a minimal reproducible example if you need! Thanks for your attention on this

@EandrewJones
Copy link
Contributor Author

EandrewJones commented Nov 5, 2022 via email

@wileymc
Copy link

wileymc commented Nov 6, 2022 via email

charlypoly added a commit to dotansimha/graphql-code-generator-community that referenced this pull request Nov 7, 2022
@EandrewJones
Copy link
Contributor Author

EandrewJones commented Nov 8, 2022

@wileymc

Are the type warnings appearing in https://github.com/populist-vote/web/blob/w/upgrade-rq/generated.ts itself or in front-end front-end query calls itself?

I'm a bit confused on where you're seeing the issue since you said the offending hooks are for queries that you'll never need an infinite query for, which suggests to me that the warnings must be in the generated.ts file itself.

Is the entire query underlined by your linter or just a specific part? I'm trying to replicate this but I can't.

@wileymc
Copy link

wileymc commented Nov 8, 2022

@wileymc

Are the type warnings appearing in https://github.com/populist-vote/web/blob/w/upgrade-rq/generated.ts itself or in front-end front-end query calls itself?

I'm a bit confused on where you're seeing the issue since you said the offending hooks are for queries that you'll never need an infinite query for, which suggests to me that the warnings must be in the generated.ts file itself.

Is the entire query underlined by your linter or just a specific part? I'm trying to replicate this but I can't.

I'm not consuming any of the offending generated hooks. The warnings are indeed in the generated file:

image

Argument of type '{ [x: string]: any; }' is not assignable to parameter of type 'Exact<{ [key: string]: never; }>'.
  'string' index signatures are incompatible.
    Type 'any' is not assignable to type 'never'.ts(2345)

However, I am noticing that one of the infinite query hooks that I am consuming is now failing at runtime when making a call for the next page.

Error: Invalid value for argument "after", expected type "String"`

The hooks usage looks like this:

  const {
    data,
    isLoading,
    error,
    fetchNextPage,
    hasNextPage,
    isFetchingNextPage,
  } = useInfinitePoliticianIndexQuery(
    "cursor",
    {
      pageSize: PAGE_SIZE,
      filter: {
        query: debouncedSearchQuery || null,
        homeState: state as State,
        politicalScope: scope as PoliticalScope,
        chambers: chamber as Chambers,
      },
    },
    {
      queryKey: [
        "politicianIndex",
        debouncedSearchQuery,
        scope,
        chamber,
        state,
      ],
      getNextPageParam: (lastPage) => {
        if (!lastPage.politicians.pageInfo.hasNextPage) return {};
        return {
          cursor: lastPage.politicians.pageInfo.endCursor,
        };
      },
    }
  );

In debugging, i've logged out lastPage.politicians.pageInfo.endCursor and it is defined and indeed a string. May be something to do with react-query v4

Let me know if you need any other information, thank you

@EandrewJones
Copy link
Contributor Author

EandrewJones commented Nov 8, 2022

@wileymc

Type Mismatch

The issue with the type checking is that CurrentUserQuery doesn't accept any variables. So there's a type mismatch between the type definition for CurrentUserQueryVariables:
https://github.com/populist-vote/web/blob/37fb984b378497cfc7de572301287bd92ec98df4/generated.ts#L1754

and the unpacking that's happening during pagination:

        {
          ...variables,
          ...(metaData.pageParam ? { [pageParamKey]: metaData.pageParam } : {}),
        }

What confuses me here is how you're storing currentUser (client-side state) in a database and looking it up without passing any variables such as a userId to the query??

For example, I see similar logic in DeleteAccount: https://github.com/populist-vote/web/blob/37fb984b378497cfc7de572301287bd92ec98df4/generated.ts#L1947

How does the back end know which account to delete if you're not passing a key of some sort?

I'm definitely missing something important here.

Fix for the broken calls

However, I am noticing that one of the infinite query hooks that I am consuming is now failing at runtime when making a call for the next page.

Error: Invalid value for argument "after", expected type "String"`

The hooks usage looks like this:

  const {
    data,
    isLoading,
    error,
    fetchNextPage,
    hasNextPage,
    isFetchingNextPage,
  } = useInfinitePoliticianIndexQuery(
    "cursor",
    {
      pageSize: PAGE_SIZE,
      filter: {
        query: debouncedSearchQuery || null,
        homeState: state as State,
        politicalScope: scope as PoliticalScope,
        chambers: chamber as Chambers,
      },
    },
    {
      queryKey: [
        "politicianIndex",
        debouncedSearchQuery,
        scope,
        chamber,
        state,
      ],
      getNextPageParam: (lastPage) => {
        if (!lastPage.politicians.pageInfo.hasNextPage) return {};
        return {
          cursor: lastPage.politicians.pageInfo.endCursor,
        };
      },
    }
  );

In debugging, i've logged out lastPage.politicians.pageInfo.endCursor and it is defined and indeed a string. May be something to do with react-query v4

Let me know if you need any other information, thank you

Well this one is more apparent: your getNextPageParam function isn't just returning lastPage.poiticians.pageInfo.endCursor, it's returning an object with a cursor field. You only need to return lastPage.poiticians.pageInfo.endCursor and the hook should work.

      getNextPageParam: (lastPage) => {
        if (!lastPage.politicians.pageInfo.hasNextPage) return ''; // Might need to change this to undefined or change the logic so this callback isn't even triggered unless hasNextPage is true
        return lastPage.politicians.pageInfo.endCursor;
      },

The way the code was changed, the infiniteQuery no longer unpacks the pageParam object ...(metaData.pageParam ?? {}) instead it simply expects the value for the pageParamKey.

Originally, I thought this was the issue, but now I believe all I needed to fix was the missing pageParamKey argument in the custom-fetcher generation files...

@blaenk
Copy link

blaenk commented Nov 10, 2022

Also experiencing this issue. Is there already an issue to track it? Not sure if it's dotansimha/graphql-code-generator-community#227

@EandrewJones
Copy link
Contributor Author

@blaenk Yes this is tracking the issue.

This is on my radar and I'll try to figure out the appropriate solution in a new PR soon.

@wileymc
Copy link

wileymc commented Nov 10, 2022

@wileymc

Type Mismatch

The issue with the type checking is that CurrentUserQuery doesn't accept any variables. So there's a type mismatch between the type definition for CurrentUserQueryVariables: https://github.com/populist-vote/web/blob/37fb984b378497cfc7de572301287bd92ec98df4/generated.ts#L1754

and the unpacking that's happening during pagination:

        {
          ...variables,
          ...(metaData.pageParam ? { [pageParamKey]: metaData.pageParam } : {}),
        }

What confuses me here is how you're storing currentUser (client-side state) in a database and looking it up without passing any variables such as a userId to the query??

For example, I see similar logic in DeleteAccount: https://github.com/populist-vote/web/blob/37fb984b378497cfc7de572301287bd92ec98df4/generated.ts#L1947

How does the back end know which account to delete if you're not passing a key of some sort?

I'm definitely missing something important here.

Fix for the broken calls

However, I am noticing that one of the infinite query hooks that I am consuming is now failing at runtime when making a call for the next page.

Error: Invalid value for argument "after", expected type "String"`

The hooks usage looks like this:

  const {
    data,
    isLoading,
    error,
    fetchNextPage,
    hasNextPage,
    isFetchingNextPage,
  } = useInfinitePoliticianIndexQuery(
    "cursor",
    {
      pageSize: PAGE_SIZE,
      filter: {
        query: debouncedSearchQuery || null,
        homeState: state as State,
        politicalScope: scope as PoliticalScope,
        chambers: chamber as Chambers,
      },
    },
    {
      queryKey: [
        "politicianIndex",
        debouncedSearchQuery,
        scope,
        chamber,
        state,
      ],
      getNextPageParam: (lastPage) => {
        if (!lastPage.politicians.pageInfo.hasNextPage) return {};
        return {
          cursor: lastPage.politicians.pageInfo.endCursor,
        };
      },
    }
  );

In debugging, i've logged out lastPage.politicians.pageInfo.endCursor and it is defined and indeed a string. May be something to do with react-query v4
Let me know if you need any other information, thank you

Well this one is more apparent: your getNextPageParam function isn't just returning lastPage.poiticians.pageInfo.endCursor, it's returning an object with a cursor field. You only need to return lastPage.poiticians.pageInfo.endCursor and the hook should work.

      getNextPageParam: (lastPage) => {
        if (!lastPage.politicians.pageInfo.hasNextPage) return ''; // Might need to change this to undefined or change the logic so this callback isn't even triggered unless hasNextPage is true
        return lastPage.politicians.pageInfo.endCursor;
      },

The way the code was changed, the infiniteQuery no longer unpacks the pageParam object ...(metaData.pageParam ?? {}) instead it simply expects the value for the pageParamKey.

Originally, I thought this was the issue, but now I believe all I needed to fix was the missing pageParamKey argument in the custom-fetcher generation files...

Thanks for the info on the infiniteQuery hook, I will give that a shot later today. Per my getCurrentUser and deleteAccount queries, they do not require any variables to be be passed because they rely on an http only cookie that is passed with the request which provides the server with all the context it needs to get/delete the user record

@EandrewJones
Copy link
Contributor Author

@wileymc Cookie-based -- got it. Makes sense.

@StampixSMO
Copy link

Interesting, after updating I'm still getting tsc complaining about:

Argument of type '{ [x: string]: any; }' is not assignable to parameter of type 'Exact<{ [key: string]: never; }>'.
  'string' index signatures are incompatible.
    Type 'any' is not assignable to type 'never'.

for infinite queries. any ideas?

Also having this error for queries without any variables

@EandrewJones
Copy link
Contributor Author

@wileymc @StampixSMO @marcmll

I spent some time looking into this. A conditional check against queryVariables that are of type Exact<{[key: string]: never}> is out of the question for various reasons.

I think a reversion to the original variable unpacking implementation is the only choice since I don't believe that was the original source of my issue and it also worked fine with your code. Original unpacking implementation:

{...variables, ...(metaData.pageParam ?? {})}

@wileymc This would mean that the fix for broken calls I gave you above would no longer work, but your prior implementation would.

If this sounds good by you, I'll go ahead and implement.

@blaenk
Copy link

blaenk commented Nov 21, 2022

This would mean that the fix for broken calls I gave you above would no longer work, but your prior implementation would.

Curious what that 'prior implementation' is? I'm interested as someone who didn't make any changes to my graphql-codegen config but can't currently generate code due to this issue. Do you mean that it will now work again without any changes, or is some change necessary and if so what is it? I personally don't mind if any changes are necessary, I'm just curious what it would entail.

Thank you so much for looking into this and working on it @EandrewJones!

@wileymc
Copy link

wileymc commented Nov 22, 2022

@wileymc @StampixSMO @marcmll

I spent some time looking into this. A conditional check against queryVariables that are of type Exact<{[key: string]: never}> is out of the question for various reasons.

I think a reversion to the original variable unpacking implementation is the only choice since I don't believe that was the original source of my issue and it also worked fine with your code. Original unpacking implementation:

{...variables, ...(metaData.pageParam ?? {})}

@wileymc This would mean that the fix for broken calls I gave you above would no longer work, but your prior implementation would.

If this sounds good by you, I'll go ahead and implement.

Thanks for taking a look! I decided to set addInfiniteQuery: false and simply wrap the generated .fetcher() for the query I need inifinite in a useInfiniteQuery

Because I don't have many queries that I need the infinite hook for, this is great for now and I don't have a preference for which implementation you go with.

@EandrewJones
Copy link
Contributor Author

@blaenk Sorry for the vague language. By "prior implementation", I was referring to the getNextPageParam in @wileymc's code which broke when I made the original change.

If you haven't made any changes to your code to try to address the change introduced in this PR, then reverting should return your existing code to working state. The original way nextPageParam variables were unpacked and passed back into the query was:

{...variables, ...(metaData.pageParam ?? {})}

I thought this was a partial source of the issue I encountered because the pageParamKey wasn't being passed in during unpacking (unless you explicitly returned an object from the getNextPageParam function with the pageParamKey e.g. return {[pageParamKey]: pageParam}), but now I believe the problem was actually a missing line of code elsewhere. So, if I revert this line of code, I think everyone should be made whole again.

@blaenk
Copy link

blaenk commented Nov 22, 2022

If you haven't made any changes to your code to try to address the change introduced in this PR, then reverting should return your existing code to working state.

If by this you mean reverting the package version, yeah I pinned it to the previously working version, I didn't mean to convey I'm currently in a broken state, I only meant that it's broken with the current version 👍🏼

So if I understand you correctly, having not made any changes other than pinning to the previously working version, once the fix is out, the intention is that it'll work by simply upgrading to it.

Thanks again for your work on this!

@EandrewJones
Copy link
Contributor Author

@blaenk That is the intention. Will update this thread when the PR is in. Might be a few days because of holidays.

@hwennnn
Copy link

hwennnn commented Jan 16, 2023

any update on this?

@EandrewJones
Copy link
Contributor Author

@hwennnn I've basically been stuck waiting on a reveiwer/admin to merge the dang PR.

The changes have been sitting and waiting. I pinged the reviewer on the PR to see if we can get this in.

@blaenk
Copy link

blaenk commented Jan 17, 2023

Looks like that other PR you mentioned is now merged. Nice work!

@EandrewJones
Copy link
Contributor Author

EandrewJones commented Jan 17, 2023 via email

@blaenk
Copy link

blaenk commented Jan 17, 2023

It does appear to work for me, thanks again for your contribution on this!

@EandrewJones
Copy link
Contributor Author

EandrewJones commented Jan 17, 2023 via email

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.

None yet

8 participants