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

(vue) - correct typescript types to support variables that are individually references #2274

Closed
BenoitRanque opened this issue Feb 13, 2022 · 7 comments · Fixed by #2608
Closed
Labels
future 🔮 An enhancement or feature proposal that will be addressed after the next release help wanted ⛏️ Extra attention is needed needs more info ✋ A question or report that needs more info to be addressable

Comments

@BenoitRanque
Copy link

BenoitRanque commented Feb 13, 2022

Per the documentation, variables may be reactive and urql will refresh queries if their value changes.
This works as expected. However the typescript types are incorect.

Currently the only typing supported is the entire variables object being a reference.
Therefore the following is not supported at the typescript layer (but actually works!)

import { ref } from 'vue';
import { useQuery } from '@urql/vue';
const limit = ref(10);
const result = useQuery<any, { limit: number }>({
  query: `
        query ($$limit: Int!) {
          todos(limit: $limit) {
            id
            title
          }
        }
      `,
  variables: { limit },
});

The obvious answer here is that I should type my variables to explicitly be { limit: Ref<number> } instead.
This becomes less obvious when code generation tools like graphql-codegen are involved.

The question then becomes: should the typing in the this library be changed to reflect it's capabilities and wrap each key of the variables object with a MaybeRef ? Or is this something that needs to change in the grapqhl-codegen plugin?
The grapqhl-codegen plugin generates types that correspond to the grapqhl query, whereas the ability to wrap variables in refs is a urql capability. Given that there is already a wrapper type for variables, I think this needs to be here.

The change should happen here
If the type for variables is changed like so, it would solve the issue.

export interface UseQueryArgs<T = any, V = object> {
    query: MaybeRef<TypedDocumentNode<T, V> | DocumentNode | string>;
    variables?: MaybeRef<{ [K in keyof V]: MaybeRef<V[K]> }>;
    requestPolicy?: MaybeRef<RequestPolicy>;
    context?: MaybeRef<Partial<OperationContext>>;
    pause?: MaybeRef<boolean>;
}

If this looks like an acceptable solution I can PR the change.
Note this would not support refs nested at an arbitrary depth within the variables object, which I believe are technically supported thanks to #1151. I'm not sure what the typing to support that looks like.

urql version & exchanges: 0.6.1

@BenoitRanque BenoitRanque added the bug 🐛 Oh no! A bug or unintented behaviour. label Feb 13, 2022
@kitten
Copy link
Member

kitten commented Feb 16, 2022

Hm, I'm not quite sure where in the documentation we talk about that, but I'm not sure whether that's actually supported 😅 I unfortunately don't remember all the mechanics of Vue's reactivity system, but I was previously assuming that variables would have to be combined into a single ref using computed 🤔

@kitten kitten added future 🔮 An enhancement or feature proposal that will be addressed after the next release help wanted ⛏️ Extra attention is needed needs more info ✋ A question or report that needs more info to be addressable and removed bug 🐛 Oh no! A bug or unintented behaviour. labels Feb 16, 2022
@kitten kitten changed the title vue-urql: correct typescript types to support variables that are individually references (vue) - correct typescript types to support variables that are individually references Feb 16, 2022
@BenoitRanque
Copy link
Author

This is supported according to #1151 and my own testing.
The behavior works as expected, only the typing is an issue.

Currently if the user cares about typing they must wrap the variables in a computed to get a ref as you stated:

import { ref } from 'vue';
import { useQuery } from '@urql/vue';
const limit = ref(10);
const variables = computed(() => ({ limit }))
const result = useQuery<any, { limit: number }>({
  query: `
        query ($limit: Int!) {
          todos(limit: $limit) {
            id
            title
          }
        }
      `,
  variables,
});

This extra step is only required due to how the typing works.
If the types are updated this becomes possible:

import { ref } from 'vue';
import { useQuery } from '@urql/vue';
const limit = ref(10);
const result = useQuery<any, { limit: number }>({
  query: `
        query ($limit: Int!) {
          todos(limit: $limit) {
            id
            title
          }
        }
      `,
  variables: { limit },
});

Which in my opinion is cleaner and more natural

@BenoitRanque
Copy link
Author

Edit: fixed missing link to documentation in the original comment, sorry about that.
Here is the relevant passage from the documentation:

All inputs that are passed to useQuery may also be reactive state. This means that both the inputs and outputs of useQuery are reactive and may change over time.

This example from the same documentation page will actually throw an error in typescript,
but otherwise works.

<template>
  <ul v-if="data">
    <li v-for="todo in data.todos">{{ todo.title }}</li>
  </ul>
  <button @click="from += 10">Next Page</button>
</template>

<script>
import { useQuery } from '@urql/vue';

export default {
  setup() {
    const from = ref(0);

    const result = useQuery({
      query: `
        query ($from: Int!, $limit: Int!) {
          todos(from: $from, limit: $limit) {
            id
            title
          }
        }
      `,
      variables: { from, limit: 10 }
    });

    return {
      from,
      data: result.data,
    };
  }
};
</script>

@BenoitRanque
Copy link
Author

Bumping this, @kitten would you like me to go ahead and PR this change?

@rmzr7
Copy link

rmzr7 commented Apr 13, 2022

Bumping this as well — thoughts @kitten ?

@kitten
Copy link
Member

kitten commented May 24, 2022

Sorry, I didn't see the pings 😅 I have nothing against looking at some code for this

@benkroeger
Copy link

I usually use reactive instead of computed to satisfy typescript

import { ref, reactive } from 'vue';
import { useQuery } from '@urql/vue';
const limit = ref(10);
const result = useQuery<any, { limit: number }>({
  query: `
        query ($limit: Int!) {
          todos(limit: $limit) {
            id
            title
          }
        }
      `,
  variables: reactive({ limit }),
});

For some reason, the types are not compatible when using shallowReactive instead. So I hope the PR for this issue will also work for shallowReactive.

Thanks everyone for your efforts

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
future 🔮 An enhancement or feature proposal that will be addressed after the next release help wanted ⛏️ Extra attention is needed needs more info ✋ A question or report that needs more info to be addressable
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants