Skip to content

Conversation

rliljest
Copy link

@rliljest rliljest commented Nov 12, 2024

Description

Addresses potential issues stemming from mismatched generated keys.

Related #866

Type of change

Please delete options that are not relevant.

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

NOTE: Does have a fairly minor breaking change in case clients were hard-coding keys (not using generated helpers) and specifying "exact" react-query key matching

How Has This Been Tested?

Automated tests have been updated to accommodate this change

Test Environment:

  • OS: macOS
  • @graphql-codegen/typescript-react-query: 6.1.0

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

Copy link

changeset-bot bot commented Nov 12, 2024

🦋 Changeset detected

Latest commit: 761b305

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

@rliljest
Copy link
Author

Is there anything that I'm missing to get this PR approved and merged? The issue that this resolves persists

saihaj
saihaj previously approved these changes Sep 15, 2025
@saihaj saihaj dismissed their stale review September 15, 2025 12:55

need someone else to give feedback

@saihaj
Copy link
Collaborator

saihaj commented Sep 15, 2025

@TkDodo thoughts?

@TkDodo
Copy link

TkDodo commented Sep 16, 2025

looking at the linked issue, if you want to treat

useDummyQuery()

and

useDummyQuery({})

the same, I guess this change is what you want. A real-life example would be helpful though. Like, why isn't the argument for useDummyQuery required so that there is only one syntax to achieve the same thing?

This may be a breaking change if users were invalidating based on exact query key matches without using the .getKey helper, although that seems unlikely

agree with that sentiment, too.

) => {
return useQuery<CurrentUserForProfileQuery, TError, TData>(
variables === undefined ? ['CurrentUserForProfile'] : ['CurrentUserForProfile', variables],
variables === undefined ? ['CurrentUserForProfile', {}] : ['CurrentUserForProfile', variables],
Copy link

Choose a reason for hiding this comment

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

why not default variables to {} to avoid the extra check:

variables?: CurrentUserForProfileQueryVariables = {},

return useQuery(['CurrentUserForProfile', variables])

Copy link
Author

Choose a reason for hiding this comment

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

Updated and rebased changes to follow your suggestion

@rliljest
Copy link
Author

... why isn't the argument for useDummyQuery required so that there is only one syntax to achieve the same thing?

That would be another valid solution to the underlying issue. The main reason why I took the approach I did in this MR was to avoid breaking changes by forcing users to supply an empty object when they did not previously.

Another thing to consider is that currently in the codegen there is no differentiation between a query that takes no arguments vs. a query that takes no required arguments. So that approach would require users to provide an empty object on queries that do not take any arguments at all, which seems like an unnecessary breaking change

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.

3 participants