feat: enable useQuery etc function outside of setup and outside component #156

GavinXue · 2022-03-30T17:34:29Z

I'd love to use useQuery, useMutation, but we only can define it in setup. so I extended it to use outside of setup and component. The idea to extends comes from pinia.

I create a sampe here: https://stackblitz.com/edit/vue-9ebjy6?file=src/App.vue , and this commit should enable it.

something should be notice:

add a new choice param(manualClient) in useQuery etc, to enable these function outside of component. eg: before app mounted. We can manually send the created client to enable it.
when useSubscription outside of setup, due to unable register beforeUnmounted, It is recommended to pause it by user.

This commit should fix #149;

hope it will be help.

…ponent

logaretm

First, I'm terribly sorry about the delay in reviewing this. This is a great addition and a very welcome contribution.

Awesome work! and the tests are passing as well, but I would love to discuss the requests I have and also add some tests to see if this works as intended outside the setup.

So we could add tests for it in those situations:

After previous await in setup
Outside the execution of setup (in a function or whatever)

packages/villus/src/utils/common.ts

logaretm · 2022-04-14T00:46:28Z

packages/villus/src/client.ts

+export let activeClient: Client | undefined;
+
+/**
+ * Sets or unsets the active client
+ *
+ * @param client - villus client instance
+ */
+export const setActiveClient = (client: Client | undefined) => (activeClient = client);
+
+/**
+ * Get the currently active client if there is any.
+ */
+export const getActiveClient = () => (getCurrentInstance() && inject(VILLUS_CLIENT)) || activeClient;


I think we should leave out the inject logic from here and instead focus on the activeClient instance getter and setter in isolation. We can move the injection to the injectWithSelf call.

Suggested change

export let activeClient: Client | undefined;

/**

* Sets or unsets the active client

*

* @param client - villus client instance

*/

export const setActiveClient = (client: Client | undefined) => (activeClient = client);

/**

* Get the currently active client if there is any.

*/

export const getActiveClient = () => (getCurrentInstance() && inject(VILLUS_CLIENT)) || activeClient;

let activeClient: Client | undefined;

/**

* Sets or unsets the active client

*

* @param client - villus client instance

*/

export const setActiveClient = (client: Client | undefined) => (activeClient = client);

/**

* Get the currently active client if there is any.

*/

export const getActiveClient = () => activeClient;

Note that since we have a reliable getter now we can avoid exporting the active client instance.

I worry about that if we getActiveClient, we can not assure injectWithSelf (resolveClient now) will be called first. so I write like below.

export const getActiveClient = () => (getCurrentInstance() && inject(VILLUS_CLIENT)) || activeClient;

I also think if we can unite the inject to getActiveClient, just like:

export function getActiveClient(manualClient?: Client): Client { if (manualClient) { return manualClient } const vm = getCurrentInstance() as any; let client = vm && inject(VILLUS_CLIENT); if (client) { setActiveClient(client); } else { client = activeClient; } if (client === null || client === undefined) { throw new Error('Cannot detect villus Client, did you forget to call `useClient`? or you can pass a specific client to manualClient param. more details please read the documentation.'); } return client; }

what do you think? If you also think it is reasonable, I will change it and have a check where use injectWithSelf.

By the way, I think it may be better to modify the error message, because I am not good at english, I just add some words after. you can make it, then I will add it to these code.

I worry about that if we getActiveClient, we can not assure resolveClient will be called first

Yes, that is a valid concern but getActiveClient is only used inside resolveClient. So there is no way it can get called before resolveClient.

I also think if we can unite the inject to getActiveClient

Yep I see you did that in resolveClient now, it doesn't matter really. We can either say getActiveClient resolves the client or resolveClient is the one to do that. Not a big difference and I think the current implementation is fine 👍

By the way, I think it may be better to modify the error message

Sure, what do you think of this:

Cannot detect villus Client, did you forget to call `useClient`? Alternatively, you can explicitly pass a client as the `manualClient` argument.

So there is no way it can get called before resolveClient.

Yes, there is no problem now, just in case someone will contribute to the code, they will not know the logic order at first (it is implicit). and worry we will forget it maybe extends code someday.

Not a big difference and I think the current implementation is fine

Agree that's fine now, so let's make it now

Cannot detect villus Client, did you forget to call useClient? Alternatively, you can explicitly pass a client as the manualClient argument.

Clear and understandable. I will add it.

logaretm · 2022-04-14T00:48:31Z

packages/villus/src/utils/common.ts


-  const injection = inject(symbol, vm?.provides?.[symbol as any]);
-  if (injection === null || injection === undefined) {
+  if (client) setActiveClient(client);


This could be dangerous to set here. Assuming either useClient or createClient were called anywhere previously this would make this redundant, right? Unless you are mitigating something I'm not aware of.

Yes, I do have this worry at first, I think if we just create one client, it is not a problem. If we can create multi Client, due to the setActiveClient comes form current instance(If we can get vm), so setting it to default client is not a problem(I think), if who want use custom client (diff from vm), they should set a manualClient param ( this is why I want add a manual param to extend multi client.)
Maybe it's ok for one client, but you are right, If we use multi client, the order of setActiveClient anywhere may cause some error, I need some time to check it, then update this later.

I have some info, due to the solution comes from pinia, I found that we only create pinia instance once, then defineStore and then useTheDefinedStore. But when we use multi villus client, we may create multi times. and the useQuery function is not depends on client. If the setActiveClient order is not managed, it may get unexpected response.
I think when we use multi villusClient, It should call useQuery to offer a specific client ( the manualClient param.), or we can suggest user to define customer useQuery in docs, just like (it is an easy and more reliable ):

export function useCustomClientQuery (opt: QueryOptions) { return useQuery(opt, customClient) }

or villus can also have define function to extend more abilities.

by the way, I think when we can get client from current instance, set it to default and use it to query is reasonable. Developers should aware of which client provided in current instance. if not , pass a specific client is reliable (recommended). I think we can make it more clear in docs.

Above all is my simple opinion. do you have any other good idea?

So what I'm thinking is, if we set the active client there it will mess up when the user uses useQuery or useMutation when it is both outside setup and they have multiple clients. It will only work for them if they expect the last created client to be injected.

If we don't do that, the same problem still occurs. Its just we only set it once and that's it. If they use a different client and expect that to be used outside the setup it will mess up then.

So regardless of our choices here this would cause issues for people using multiple clients.
I'm willing to argue that using multiple clients at the same app isn't common and usually people stitch schemas on the backend, so this might be fine to leave as is.

Like you said the manualClient option would solve it for those users in that case, which can be documented to make sure they don't fall into that trap.

when it is both outside setup and they have multiple clients. It will only work for them if they expect the last created client to be injected.

It will use last currentInstance used client to execute query, because we may set activeClient in last currentInstance (resolveClient function). If we don't set, villus will execute with last created client. both will have some risk (if use multi client). I think last currentInstance client maybe more probability which user want choose.

So regardless of our choices here this would cause issues for people using multiple clients.

I do have the worry. but I have no more good idea now. so let's add it todo. and make a caution in documentation first to remind multi client user. Maybe someday new idea will come.

By the way, because we will call createClient in useClient function. if we useClient before query, the activeClient will be set to the right one. So I think some one can useClient first as before (Maybe).

packages/villus/src/utils/common.ts

fanckush · 2022-04-14T09:07:03Z

Can this PR also add documentation for the usage?
Thanks!

logaretm · 2022-04-15T02:34:47Z

@fanckush I will be sure to add documentation to it unless @GavinXue wants to do it.

Co-authored-by: Abdelrahman Awad <logaretm1@Gmail.com>

GavinXue · 2022-04-15T15:44:21Z

@fanckush I will be sure to add documentation to it unless @GavinXue wants to do it.

As you can see, I am not good at english 😄 , I do have think adding doc commit before, but maybe it exceed my writing english ability. So I think it would be better let you @logaretm add it. Your writing documentation is clear and easy to understand (Wonderful and thanks) . If you need some help, please let me know.

logaretm · 2022-04-16T03:22:07Z

Last thing needed I think is to change injectWithSelf to resolveClient in the useQuery, useMutation, and useSubscription files to make sure tests pass.

GavinXue · 2022-04-16T03:58:43Z

Last thing needed I think is to change injectWithSelf to resolveClient in the useQuery, useMutation, and useSubscription files to make sure tests pass.

Yes, I am waiting for your opinion before, and then I will change it ASAP.

- add resolveClient symbol argument back, in case extending someday. - modify error msg.

codecov-commenter · 2022-04-16T05:08:44Z

Codecov Report

Merging #156 (33b9ca0) into main (ca2a6b2) will increase coverage by 0.09%.
The diff coverage is 94.87%.

@@            Coverage Diff             @@
##             main     #156      +/-   ##
==========================================
+ Coverage   94.78%   94.88%   +0.09%     
==========================================
  Files          26       26              
  Lines         537      547      +10     
  Branches       99      107       +8     
==========================================
+ Hits          509      519      +10     
  Misses         28       28

Impacted Files	Coverage Δ
packages/villus/src/utils/common.ts	`88.23% <87.50%> (-4.63%)`	⬇️
packages/villus/src/client.ts	`98.55% <90.00%> (-1.45%)`	⬇️
packages/villus/src/index.ts	`100.00% <100.00%> (ø)`
packages/villus/src/useMutation.ts	`96.15% <100.00%> (-0.15%)`	⬇️
packages/villus/src/useQuery.ts	`97.01% <100.00%> (-0.05%)`	⬇️
packages/villus/src/useSubscription.ts	`96.36% <100.00%> (+3.63%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ca2a6b2...33b9ca0. Read the comment docs.

logaretm · 2022-04-16T15:56:51Z

@GavinXue Looking good 👍 there is a conflict though with the main branch. Let me know if you need help resolving it.

Other than that, I can add the documentation into this PR and a couple of tests and we can then merge it!

GavinXue · 2022-04-16T17:53:26Z

@logaretm Is it okay now? I am sorry, I am not familiar with resolving conflict.

logaretm · 2022-04-16T20:30:30Z

Yep, all good. I will work on the documentation later tonight and tag a new release with this. Thank you so much for this contribution 👍

I'm happy to merge now if you are.

GavinXue · 2022-04-17T02:14:32Z

@logaretm sure, thanks. I am glad this is useful.

By the way, I found that maybe the documentation will suggest to use getActiveClient #157 (comment). I think we should notice the getActiveClient function will not getCurrentInstance client first (it always get last active client, it has risk in some cases). and we have no error message if there is no client (I'm not sure if needed). Considering the risk, I think we can do these choice again #156 (comment) . What do you think?

logaretm · 2022-04-17T04:18:11Z

Considering the risk, I think we can do these choice again

I think you are right, since we are exposing getActiveClient to the public it may not work in a few cases as expected. So including the injection might be a good idea here. So your initial implementation actually made sense here.

So something like this would make it reliable for those who might want to call it to get access to the client being used for whatever reason.

export const getActiveClient = () => (getCurrentInstance() && inject(VILLUS_CLIENT)) || activeClient;

I don't think it should be unified with resolveClient now though, because resolveClient does a few extra things:

First it allows overriding with a custom client.
secondly it includes the same component instance in the injection (old behavior of injectWithSelf).
Third, it throws when it cannot find the client

Would say if you want to unify it, then the second behavior should be fine to include but not the first one nor the third. I'm thinking it should behave similar to Vue's getCurrentInstance.

I think this is the best value it can offer, since you don't really want it to throw unless it is critical, consider how people will use it in either case:

const client = getActiveClient();
if (!client) {
  // do something
}

This is much nicer than:

try {
const client = getActiveClient();
} catch (err) {
  // do something
}

Meanwhile it is perfectly fine for resolveClient to throw since it MUST find a client to be used, right? otherwise nothing will work.

I suggest getActiveClient be something like this:

So it might look like this eventually:

export const getActiveClient = () => {
  const vm = getCurrentInstance();
  if (!vm) {
    return activeClient;
  }

  return  inject(VILLUS_CLIENT, vm?.provides?.[VILLUS_CLIENT]) || activeClient;
}

This should address some of your concerns, What do you think?

GavinXue · 2022-04-17T07:49:15Z

@logaretm I don't think unified solution is a good idea either, it seems make something complicated, and thank you for making it clearly.

export const getActiveClient = () => {
  const vm = getCurrentInstance();
  if (!vm) {
    return activeClient;
  }

  return  inject(VILLUS_CLIENT, vm?.provides?.[VILLUS_CLIENT]) || activeClient;
}

I do think the suggested getActiveClient is good choice. but I don't understand some little change, I think it may have no difference with the initial implementation. the only difference is inject(VILLUS_CLIENT, vm?.provides?.[VILLUS_CLIENT]), this make puzzled sometimes, if the default value vm?.provides?.[VILLUS_CLIENT] exists, the inject(VILLUS_CLIENT) should be get that result (I'm not sure 100%), if so, the default value is not needed here (I think). could you please help me for a little more explaining?

If we don't need the default value of inject, there is no difference vs initial getActiveClient

export const getActiveClient = () => (getCurrentInstance() && inject(VILLUS_CLIENT)) || activeClient;

logaretm · 2022-04-17T13:46:18Z

if the default value vm?.provides?.[VILLUS_CLIENT] exists, the inject(VILLUS_CLIENT) should be get that result (I'm not sure 100%)

Not really. Currently in Vue, the default inject behavior injects from the parent tree and doesn't include the same component as the source of injection. Meaning it will be only able to inject it if it was provided in the parent.

In this example let's say we don't use the vm?.provides?.[VILLUS_CLIENT] part.

// setup
useClient();
getActiveClient();
useQuery();

both useQuery and getActiveClient won't be able to inject the same client that was just created in the same setup function. Of course the activeClient might be correctly resolved anyways because of activeClient being set.

However this will cause an issue if both the parent and child components call useClient. This would cause the child component queries to be resolved from its parent client.

So this part vm?.provides?.[VILLUS_CLIENT] makes sure we include the client created/provided by the same component. And actually to be more accurate here it should priotorize the same component client over the parent one.

export const getActiveClient = () => {
  const vm = getCurrentInstance();
  if (!vm) {
    return activeClient;
  }

  return vm.provides?.[VILLUS_CLIENT] ||  inject(VILLUS_CLIENT, activeClient);
}

GavinXue · 2022-04-17T16:44:52Z

the default inject behavior injects from the parent tree and doesn't include the same component as the source of injection. Meaning it will be only able to inject it if it was provided in the parent.

@logaretm Got it, thanks for your patient to explain clearly. Your suggest is more reliable and make sense. I've add the code to this PR. You can have a check.

It seems that the things I can do is all done. If there is any help I can do, please let me know.

logaretm · 2022-04-18T00:43:18Z

Awesome @GavinXue, no worries at all and thank you a lot for this awesome PR. I will merge it now and I will be releasing it within a couple of days to make sure I add documentation for it 👍

GavinXue added 2 commits March 29, 2022 22:24

fix: add export types for villus.d.ts

480ac65

feat: enable useQuery etc functions outside of setup and outside comm…

54b2029

…ponent

logaretm requested changes Apr 14, 2022

View reviewed changes

GavinXue and others added 4 commits April 15, 2022 22:57

Update packages/villus/src/utils/common.ts

8c0a6d3

Co-authored-by: Abdelrahman Awad <logaretm1@Gmail.com>

Update packages/villus/src/utils/common.ts

ae56b78

Co-authored-by: Abdelrahman Awad <logaretm1@Gmail.com>

Update packages/villus/src/client.ts

62d9db5

Co-authored-by: Abdelrahman Awad <logaretm1@Gmail.com>

Update packages/villus/src/utils/common.ts

df3abc8

Co-authored-by: Abdelrahman Awad <logaretm1@Gmail.com>

change to get activeClient using getter

2bd94dd

GavinXue added 5 commits April 16, 2022 12:50

update common.ts

e310018

- add resolveClient symbol argument back, in case extending someday. - modify error msg.

add a comment just in case

8dcd72d

replace injectWithSelf with resolveClient

d5fca3b

replace injectWithSelf with resolveClient

9b7f2da

replace injectWithSelf with resolveClient

1ca85f4

Merge branch 'main' into feat-outside-setup

a4c5d2f

logaretm approved these changes Apr 16, 2022

View reviewed changes

GavinXue added 2 commits April 18, 2022 00:21

update getActiveClient and remove a comment

273383f

change vm type to any to solve no provides prop

b26dcce

change VILLUS_CLIENT type as any to fix ts error

33b9ca0

logaretm merged commit 14335b3 into logaretm:main Apr 18, 2022

logaretm mentioned this pull request Apr 23, 2022

How to use in e.g. Pinia #149

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: enable useQuery etc function outside of setup and outside component #156

feat: enable useQuery etc function outside of setup and outside component #156

GavinXue commented Mar 30, 2022

logaretm left a comment •

edited

logaretm Apr 14, 2022

GavinXue Apr 15, 2022

GavinXue Apr 15, 2022

logaretm Apr 16, 2022

GavinXue Apr 16, 2022 •

edited

logaretm Apr 14, 2022

GavinXue Apr 15, 2022

GavinXue Apr 15, 2022

logaretm Apr 16, 2022

GavinXue Apr 16, 2022

GavinXue Apr 16, 2022

fanckush commented Apr 14, 2022

logaretm commented Apr 15, 2022

GavinXue commented Apr 15, 2022 •

edited

logaretm commented Apr 16, 2022

GavinXue commented Apr 16, 2022

codecov-commenter commented Apr 16, 2022 •

edited

logaretm commented Apr 16, 2022

GavinXue commented Apr 16, 2022

logaretm commented Apr 16, 2022 •

edited

GavinXue commented Apr 17, 2022

logaretm commented Apr 17, 2022

GavinXue commented Apr 17, 2022 •

edited

logaretm commented Apr 17, 2022 •

edited

GavinXue commented Apr 17, 2022

logaretm commented Apr 18, 2022

feat: enable useQuery etc function outside of setup and outside component #156

feat: enable useQuery etc function outside of setup and outside component #156

Conversation

GavinXue commented Mar 30, 2022

logaretm left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GavinXue Apr 16, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fanckush commented Apr 14, 2022

logaretm commented Apr 15, 2022

GavinXue commented Apr 15, 2022 • edited

logaretm commented Apr 16, 2022

GavinXue commented Apr 16, 2022

codecov-commenter commented Apr 16, 2022 • edited

Codecov Report

logaretm commented Apr 16, 2022

GavinXue commented Apr 16, 2022

logaretm commented Apr 16, 2022 • edited

GavinXue commented Apr 17, 2022

logaretm commented Apr 17, 2022

GavinXue commented Apr 17, 2022 • edited

logaretm commented Apr 17, 2022 • edited

GavinXue commented Apr 17, 2022

logaretm commented Apr 18, 2022

logaretm left a comment •

edited

GavinXue Apr 16, 2022 •

edited

GavinXue commented Apr 15, 2022 •

edited

codecov-commenter commented Apr 16, 2022 •

edited

logaretm commented Apr 16, 2022 •

edited

GavinXue commented Apr 17, 2022 •

edited

logaretm commented Apr 17, 2022 •

edited