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

Urql 4 now have more strict types for variables #329

Merged
merged 2 commits into from
Sep 7, 2023
Merged

Conversation

mlesin
Copy link
Contributor

@mlesin mlesin commented Apr 12, 2023

Description

Specify operation variable types explicitly in useQuery generated functions to be able to work with Urql 4.

Related #328

Type of change

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

Test Environment:

  • OS: Linux
  • @graphql-codegen/typescript-vue-urql:
  • NodeJS: 18.14.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

@changeset-bot
Copy link

changeset-bot bot commented Apr 12, 2023

🦋 Changeset detected

Latest commit: 1fb6557

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-vue-urql Minor

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

@mlesin
Copy link
Contributor Author

mlesin commented Apr 13, 2023

I found that this is probably a breaking change: in case there are no variables needed for the query, the user must now pass an empty object

@@ -98,8 +98,8 @@ export function use${operationName}<R = ${operationResultType}>(options: Omit<Ur
}

return `
export function use${operationName}(options: Omit<Urql.Use${operationType}Args<never, ${operationVariablesTypes}>, 'query'> = {}) {

Choose a reason for hiding this comment

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

Even leaving the default in place works for me in all cases, no matter if I

  • have required variables,
  • have only optional variables,
  • have no variables at all.

With @urql/vue ^1.1.1, the only required change is your change in line 102.

I would prefer to leave the default (line 101) in place, to make it a non-breaking change for current users.

Choose a reason for hiding this comment

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

I use typescript version 5.

Choose a reason for hiding this comment

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

I'M SORRY: I WAS WRONG. My variables were marked as optional without my knowledge. Your version is right and also working for required variables!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can we merge and publish these changes?

Choose a reason for hiding this comment

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

This PR is stale for more than a month now, can we get this merged or provide reasons for not merging it?

Copy link

Choose a reason for hiding this comment

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

do you know when this PR can be merged ? ... since 12 april ...

Copy link

@Dodobibi Dodobibi Aug 19, 2023

Choose a reason for hiding this comment

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

Variables & variables fields maybe ref :

-export function use${operationName}(options: Omit<Urql.Use${operationType}Args<never, ${operationVariablesTypes}>, 'query'> = {}) {
-  return Urql.use${operationType}<${operationResultType}>({ query: ${documentVariableName}, ...options });
+type MaybeRef<T> = T | Ref<T>; 
+export function use${operationName}(options: Omit<Urql.Use${operationType}Args<never, MaybeRef<{[K in keyof ${operationVariablesTypes}]: MaybeRef<${operationVariablesTypes}[K]>}>>, 'query'>) {
+  return Urql.use${operationType}<${operationResultType}, ${operationVariablesTypes}>({ query: ${documentVariableName}, ...options });

@lucastraba
Copy link

lucastraba commented Aug 2, 2023

Please, please, please, give this PR some love. It's very simple and fixes a very critical issue. The plugin is unusable without this fix.

@ognjenmarcheta
Copy link

@dotansimha @saihaj @n1ru4l
Can we please merge this, many people are waiting for this fix.

@saihaj
Copy link
Collaborator

saihaj commented Sep 5, 2024

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.

8 participants