Vue query improve reactive support #852

gkweb · 2023-05-06T14:43:52Z

Status

READY/WIP/HOLD

READY

Description

Addresses: #851

A few sentences describing the overall goals of the pull request's commits.

Improves vue reactivity support.

Related PRs

List related PRs against other branches:

branch	PR
other_pr_production	link
other_pr_master	link

Todos

Tests
Documentation
Changelog Entry (unreleased)

Steps to Test or Reproduce

Outline the steps to test or reproduce the PR here.

See the sample update, I've expanded on the sample by dynamically updating the list to load in a single pet.

> git pull --prune
> git checkout <branch>
> grunt jasmine

vercel · 2023-05-06T14:43:54Z

Someone is attempting to deploy a commit to a Personal Account owned by @anymaniax on Vercel.

@anymaniax first needs to authorize it.

…neric

gkweb · 2023-05-07T00:57:52Z

@anymaniax - Now ready for review, remaining type fixes added. Thank you for your consideration.

Maxim-Mazurok · 2023-05-08T07:55:52Z

This seems to work quite well for me

anymaniax · 2023-05-08T07:58:18Z

I see that the tests are not passing. There is a problem for react-query and svelte-query

gkweb · 2023-05-08T08:01:11Z

I see that the tests are not passing. There is a problem for react-query and svelte-query

@anymaniax - Are you available on a chat anywhere? Or can you share the output of failure here? I can take a look then! Many Thanks

anymaniax · 2023-05-08T08:06:40Z

I try to be available on the Orval discord if you need to talk to me. You will see the problem if in the project you run the tests in the tests folder you just need to install the dependencies in it and then yarn generate && yarn build

gkweb · 2023-05-08T08:07:45Z

packages/query/src/index.ts

      props.filter((prop) => prop.type !== GetterPropType.HEADER),
      'implementation',
    );

+    if (OutputClient.VUE_QUERY) {


@anymaniax isOutputClient.VUE_QUERY a reliable way to detect if the runner is vue only?

Ta

Yes it's the only way to know it

gkweb · 2023-05-08T08:09:25Z

I see that the tests are not passing. There is a problem for react-query and svelte-query

@anymaniax - Are you available on a chat anywhere? Or can you share the output of failure here? I can take a look then! Many Thanks

All good I see this now!

Left a comment above. Will join discord. Ta

Maxim-Mazurok · 2023-05-09T06:05:01Z

As I've demonstrated in #851 (comment) the query isn't being refetched when selected petId changes.
This seems to fix the issue but I hope there's a better way:

import { watch } from 'vue';
watch(petId, () => {
  petQuery.refetch();
});

Maxim-Mazurok · 2023-05-09T07:16:15Z

I think I figured it out.
In getShowPetByIdQueryKey you're doing unref() so that you can return a string.
If we instead return an array (which is also a good key option for Vue Query as far as I understand) - then array values will still be reactive.
So this works without manual watcher:

export const getShowPetByIdQueryKey = (petId: MaybeRef<string>, version = 1) =>
-  [`/v${unref(version)}/pets/${unref(petId)}`] as const;
+  [`/v`, version, `/pets/`, petId] as const;

gkweb · 2023-05-09T09:19:07Z

I think I figured it out.

In getShowPetByIdQueryKey you're doing unref() so that you can return a string.

If we instead return an array (which is also a good key option for Vue Query as far as I understand) - then array values will still be reactive.

So this works without manual watcher:
export const getShowPetByIdQueryKey = (petId: MaybeRef<string>, version = 1) =>

-  [`/v${unref(version)}/pets/${unref(petId)}`] as const;

+  [`/v`, version, `/pets/`, petId] as const;

Thanks @Maxim-Mazurok

I agree with the query key solution as Vue-query handles it and unwraps automatically. Let me take a look when I get some time. My first suggestion was to update queryKey also. Cheers

Maxim-Mazurok · 2023-05-11T01:45:41Z

Found another small issue:

This seems to fix that:

diff --git a/packages/query/src/index.ts b/packages/query/src/index.ts
index 4f62c9a..e126713 100644
--- a/packages/query/src/index.ts
+++ b/packages/query/src/index.ts
@@ -402,10 +402,11 @@ const generateQueryRequestFunction = (
     hasSignal,
   });
 
-  return `export const ${operationName} = (\n    ${toObjectString(
-    props,
-    'implementation',
-  )} ${optionsArgs} ): Promise<AxiosResponse<${
+  const queryProps = isVue(outputClient)
+    ? vueWrapTypeWithMaybeRef(toObjectString(props, 'implementation'))
+    : toObjectString(props, 'implementation');
+
+  return `export const ${operationName} = (\n    ${queryProps} ${optionsArgs} ): Promise<AxiosResponse<${
     response.definition.success || 'unknown'
   }>> => {${bodyForm}
     return axios${

Maxim-Mazurok · 2023-05-11T02:15:13Z

Found another option which also works:

export const getShowPetByIdQueryKey = (petId: MaybeRef<string>, version = 1) =>
  computed(() => [`/v${unref(version)}/pets/${unref(petId)}`]);

Maxim-Mazurok · 2023-05-11T03:05:31Z

Implemented following option based on this PR

[`/v`, version, `/pets`]

gkweb#1

Array reactivity

gkweb · 2023-05-12T15:31:00Z

@anymaniax tests are now passing for this, so awaiting your review.

Maxim-Mazurok · 2023-05-15T00:47:12Z

[/v, version, /pets]

I did it this way because I wasn't sure how Vue Query hashes the key, I thought that if it does [].join("") for example - then /v1/air/plane would have the same key as /v1/airplane which would be a mistake.

Checking out their code, I've created a test to confirm that we can drop / from the keys and not get collisions. Keys are hashed using JSON.stringify() so commas will serve as delimiters, no need for / indeed. Here's the test:

function isPlainObject(o) {
  if (!hasObjectPrototype(o)) {
    return false
  }

  // If has modified constructor
  const ctor = o.constructor
  if (typeof ctor === 'undefined') {
    return true
  }

  // If has modified prototype
  const prot = ctor.prototype
  if (!hasObjectPrototype(prot)) {
    return false
  }

  // If constructor does not have an Object-specific method
  if (!prot.hasOwnProperty('isPrototypeOf')) {
    return false
  }

  // Most likely a plain Object
  return true
}

function hasObjectPrototype(o) {
  return Object.prototype.toString.call(o) === '[object Object]'
}

function hashQueryKey(queryKey) {
  return JSON.stringify(queryKey, (_, val) =>
    isPlainObject(val)
      ? Object.keys(val)
          .sort()
          .reduce((result, key) => {
            result[key] = val[key]
            return result
          }, {})
      : val,
  )
}

console.log(hashQueryKey(["v", 1, "air", "plane"])) // prints ["v",1,"air","plane"]
console.log(hashQueryKey(["v", 1, "airplane"])) // prints ["v",1,"airplane"]

Maxim-Mazurok · 2023-05-15T01:59:28Z

Just tested this on my project and it generates everything well and works well, so LGTM.

There's another reactivity issue tho, now with enabled option.

Basically, we have this now: enabled: !!myArgument and since myArgument is a ref/proxy - enabled is always true.

I've solved it by doing this:

- return { queryKey, queryFn, enabled: !!myArgument, ...queryOptions };
+ return { queryKey, queryFn, enabled: computed(() => !!unref(myArgument)), ...queryOptions };

Seems to work fine. I'll submit PR

gkweb · 2023-05-15T02:09:45Z

samples/vue-query/src/api/endpoints/petstoreFromFileSpecWithTransformer.ts

@@ -49,7 +49,7 @@ export const listPets = (
 export const getListPetsQueryKey = (
  params?: MaybeRef<ListPetsParams>,
  version = 1,
-) => [`/v`, version, `/pets`, ...(params ? [params] : [])] as const;
+) => ['v', version, 'pets', ...(params ? [params] : [])] as const;


@Maxim-Mazurok

RE: I did it this way because I wasn't sure how Vue Query hashes the key, I thought that if it does [].join("") for example - then /v1/air/plane would have the same key as /v1/airplane which would be a mistake.

I've changed what functionality you had here previously. Is this what you're describing as a requirement? (forward slash removed). Any string concatenation with variables even in the pathname should be cut up and recognised as a var.
eg: /something${var}/pets/${petId} will explode to ['something',var,'pets',petId]

It's all good now, thank you. I was just confirming that we can get rid of slashes and keep arrays just like you did, all good currently 👍

gkweb · 2023-05-15T02:15:03Z

Just tested this on my project and it generates everything well and works well, so LGTM.

There's another reactivity issue tho, now with enabled option.

Basically, we have this now: enabled: !!myArgument and since myArgument is a ref/proxy - enabled is always true.

I've solved it by doing this:
- return { queryKey, queryFn, enabled: !!myArgument, ...queryOptions };
+ return { queryKey, queryFn, enabled: computed(() => !!unref(myArgument)), ...queryOptions };
Seems to work fine. I'll submit PR

vue-query supports ref here I'm pretty sure, So this only needs to be changed to: enabled: myArgument right? - If you need to expand on that just pass in your own enabled implementation?

Maxim-Mazurok · 2023-05-15T02:16:42Z

packages/query/src/index.ts

@@ -849,7 +878,7 @@ export const ${camel(
    `${operationPrefix}-${type}`,
  )}(${queryOptionsVarName}) as ${returnType};

-  query.queryKey = ${queryOptionsVarName}.queryKey;
+  query.queryKey = ${queryOptionsVarName}.queryKey as QueryKey;


This seems to apply to samples/react-query/hook-mutator/endpoints.ts as well as vue one, not sure if that was your intention.

Maxim-Mazurok · 2023-05-15T02:27:27Z

vue-query supports ref here I'm pretty sure

It doesn't seem to be the case...

So this only needs to be changed to: enabled: myArgument right?

Nope, that doesn't work, it's being evaluated to true when I pass ref(undefined).

If you need to expand on that just pass in your own enabled implementation?

I guess I could... But I rather patch it up in the generator, see gkweb#2 We don't have to include it in this PR tho if you'd like to do it separately. Seems like a small fix to me tho.

anymaniax · 2023-05-17T10:13:44Z

Hello guys, thanks for the work can I do something here? is the pull request ready?

gkweb · 2023-05-17T10:22:00Z

Hello guys, thanks for the work can I do something here? is the pull request ready?

Hey @anymaniax - yeah any subsequent issue can be fixed in additional pull requests.
There are some more improvements which can still be done here but this has paved the way to improving reactivity.

anymaniax · 2023-05-17T10:27:47Z

Just did a first tour. Insane work thanks guys ❤️

anymaniax · 2023-05-19T08:51:37Z

samples/vue-query/src/components/pet.vue

+import { useShowPetById } from '../api/endpoints/petstoreFromFileSpecWithTransformer';
+
+const props = defineProps<{ petId: string }>();
+const petId = computed(() => props.petId);


the computed is not really need here right?

The props is a reactive object. Wrapping in the computed can help to demonstrate reactivity of the individual prop though?

https://vuejs.org/guide/extras/reactivity-transform.html#reactive-props-destructure

Let me know if you want this changed?

no it was to be sure

anymaniax

Thanks 🎉

AlmarAubel · 2023-05-23T06:15:05Z

Great work, any idea when you will release this to NPM?

anymaniax · 2023-05-23T10:54:40Z

This week. I want to add one more pull request before the release.

anymaniax · 2023-05-23T11:09:17Z

Changed my mind it's released right now

* fix: correct `jsStringEscape` import * feat(namedParameters): add global `useNamedParameters` option * feat(namedParameters): add NAMED_PATH_PARAMS prop type * test(axios): add named parameters test * test(angular): add named parameters test * test(react-query): add named parameters test * fix(query): introduce destructured usage * test(svelte-query): add named parameters test * test(vue-query): add named parameters test * fix(vue-query): rework MaybeRef logic * fix(swr): introduce destructured usage * test(swr): add named parameters test * chore(cra): update react-scripts * fix(angular-app): update sample * fix(react-app): update sample * fix(react-app-with-swr): update sample * fix(react-query/basic-app): update sample * fix(react-query/custom-client-app): update sample * fix(react-query/form-data-app): update sample * fix(react-query/form-data-mutator-app): update sample * fix(react-query/form-url-encoded-mutator-app): update sample * fix(react-query/hook-mutator-app): update sample * fix(svelte-query): update sample * fix(vue-query-app): update sample * fix(namedParameters): optional arg when all params are optional

gkweb added 4 commits May 7, 2023 00:25

feat(samples): improve vue query sample and use param

8f681a5

feat(samples): move vue sample to use @tanstack/vue-query

29030e7

fix(query): vue make route path props reactive

72ba40f

fix(samples): vue query update api generation sample

3df0f97

gkweb added 3 commits May 7, 2023 10:27

feat(query): vue query wrap function prop types with MaybeRef generic

93f5f8c

feat(query): vue query wrap more function prop types with MaybeRef ge…

dfb947b

…neric

feat(samples): update vue-query api sample

fd13ce5

feat(samples): vue-query remove todo comment

f25031f

gkweb commented May 8, 2023

View reviewed changes

gkweb added 2 commits May 8, 2023 19:03

fix(query): improve vue detection and added utility

6574811

fix(tests): added vue to tests for tsc

cd48c50

fix: ts

2c2297d

Maxim-Mazurok added 2 commits May 11, 2023 12:55

feat: getRouteArray

d80fca8

fix: oops

35df6dd

gkweb added 3 commits May 13, 2023 00:38

Merge pull request #1 from Maxim-Mazurok/master

f212569

Array reactivity

feat(core): getRouteAsArray for use with vue query key

e3c3114

Merge branch 'master' of github.com:gkweb/orval

115937d

gkweb added 3 commits May 13, 2023 01:17

feat(query): vue reactive query key and query key type fix

c1282e4

feat(samples): vue-query samples update after api generate

59e8b26

feat(query): consistent vue util names

6ae336c

gkweb commented May 15, 2023

View reviewed changes

Maxim-Mazurok reviewed May 15, 2023

View reviewed changes

fix(query): query key assertion for vue only

2d83011

Maxim-Mazurok mentioned this pull request May 15, 2023

fix(query): enabled reactivity fix gkweb/orval#2

Closed

Maxim-Mazurok mentioned this pull request May 16, 2023

Evaluation report (with Vue 3 + Vue Query) rametta/rapini#40

Closed

anymaniax reviewed May 19, 2023

View reviewed changes

anymaniax approved these changes May 19, 2023

View reviewed changes

anymaniax merged commit 64070a6 into orval-labs:master May 19, 2023

Maxim-Mazurok mentioned this pull request Nov 29, 2023

vue-query add reactive support #851

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Vue query improve reactive support #852

Vue query improve reactive support #852

gkweb commented May 6, 2023 •

edited

Loading

vercel bot commented May 6, 2023

gkweb commented May 7, 2023

Maxim-Mazurok commented May 8, 2023

anymaniax commented May 8, 2023

gkweb commented May 8, 2023

anymaniax commented May 8, 2023

gkweb May 8, 2023

anymaniax May 8, 2023

gkweb commented May 8, 2023

Maxim-Mazurok commented May 9, 2023 •

edited

Loading

Maxim-Mazurok commented May 9, 2023 •

edited

Loading

gkweb commented May 9, 2023

Maxim-Mazurok commented May 11, 2023 •

edited

Loading

Maxim-Mazurok commented May 11, 2023

Maxim-Mazurok commented May 11, 2023 •

edited

Loading

gkweb commented May 12, 2023

Maxim-Mazurok commented May 15, 2023

Maxim-Mazurok commented May 15, 2023 •

edited

Loading

gkweb May 15, 2023 •

edited

Loading

Maxim-Mazurok May 15, 2023

gkweb commented May 15, 2023

Maxim-Mazurok May 15, 2023

Maxim-Mazurok commented May 15, 2023

anymaniax commented May 17, 2023

gkweb commented May 17, 2023

anymaniax commented May 17, 2023

anymaniax May 19, 2023

gkweb May 19, 2023

anymaniax May 19, 2023 •

edited

Loading

anymaniax left a comment

AlmarAubel commented May 23, 2023

anymaniax commented May 23, 2023 •

edited

Loading

anymaniax commented May 23, 2023

Vue query improve reactive support #852

Vue query improve reactive support #852

Conversation

gkweb commented May 6, 2023 • edited Loading

Status

Description

Related PRs

Todos

Steps to Test or Reproduce

vercel bot commented May 6, 2023

gkweb commented May 7, 2023

Maxim-Mazurok commented May 8, 2023

anymaniax commented May 8, 2023

gkweb commented May 8, 2023

anymaniax commented May 8, 2023

gkweb May 8, 2023

Choose a reason for hiding this comment

anymaniax May 8, 2023

Choose a reason for hiding this comment

gkweb commented May 8, 2023

Maxim-Mazurok commented May 9, 2023 • edited Loading

Maxim-Mazurok commented May 9, 2023 • edited Loading

gkweb commented May 9, 2023

Maxim-Mazurok commented May 11, 2023 • edited Loading

Maxim-Mazurok commented May 11, 2023

Maxim-Mazurok commented May 11, 2023 • edited Loading

gkweb commented May 12, 2023

Maxim-Mazurok commented May 15, 2023

Maxim-Mazurok commented May 15, 2023 • edited Loading

gkweb May 15, 2023 • edited Loading

Choose a reason for hiding this comment

Maxim-Mazurok May 15, 2023

Choose a reason for hiding this comment

gkweb commented May 15, 2023

Maxim-Mazurok May 15, 2023

Choose a reason for hiding this comment

Maxim-Mazurok commented May 15, 2023

anymaniax commented May 17, 2023

gkweb commented May 17, 2023

anymaniax commented May 17, 2023

anymaniax May 19, 2023

Choose a reason for hiding this comment

gkweb May 19, 2023

Choose a reason for hiding this comment

anymaniax May 19, 2023 • edited Loading

Choose a reason for hiding this comment

anymaniax left a comment

Choose a reason for hiding this comment

AlmarAubel commented May 23, 2023

anymaniax commented May 23, 2023 • edited Loading

anymaniax commented May 23, 2023

gkweb commented May 6, 2023 •

edited

Loading

Maxim-Mazurok commented May 9, 2023 •

edited

Loading

Maxim-Mazurok commented May 9, 2023 •

edited

Loading

Maxim-Mazurok commented May 11, 2023 •

edited

Loading

Maxim-Mazurok commented May 11, 2023 •

edited

Loading

Maxim-Mazurok commented May 15, 2023 •

edited

Loading

gkweb May 15, 2023 •

edited

Loading

anymaniax May 19, 2023 •

edited

Loading

anymaniax commented May 23, 2023 •

edited

Loading