-
-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
feat(vue-query): support injectable contexts #5886
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Ignored Deployment
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit f81488e:
|
☁️ Nx Cloud ReportCI is running/has finished running commands for commit f81488e. As they complete they will appear below. Click to see the status, the terminal output, and the build insights. 📂 See all runs for this branch ✅ Successfully ran 1 targetSent with 💌 from NxCloud. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would prefer to not introduce this in the current v4.x version if it requires other libraries to be updated as well.
Even though bumping two packages minor version should not be a problem i would prefer to avoid people updating vue-query
realize that it does not work via compilation errors and then trying to figure out what is wrong.
v5 is in beta already, so i guess it would be more appropriate to introduce this feature there.
This will have a proper message in migration guide
and will encourage people to update to the latest version if they need this feature (which is a win-win).
As for
The only thing that I think is left to do is to add extra warnings in some APIs that rely on the Reactivity API (ref, watch, onScopeDispose etc) by checking if there is a current effect scope. I think this was previously achieved by checking for a current instance within the useQueryClient which seems to be used in functions relying on those functions. But since it wasn't tested, I didn't add them. Let me know if I should add them.
This is true, but we had an escape hatch in place, where you could provide queryClient
as a parameter to useQuery
which will in result skip call to useQueryClient
.
Now when you mentioned it it seems that if someone would use it without effectScope()
it would leak. So check for it on top of every composable just before const client = queryClient || useQueryClient()
would prevent that from happening.
!hasInjectionContext() || | ||
// ensures `ref()`, `onScopeDispose()` and other APIs can be used | ||
!getCurrentScope() | ||
) { | ||
throw new Error('vue-query hooks can only be used inside setup() function.') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess we would need to refine this message to be in line with the logic.
Maybe something along
'vue-query hooks can only be used inside setup() function or functions that support injection context. In case a function with injection context is used, make sure that vue-query hook runs in the effectScope().'
?
Or we could split the check to provide more fine-grained error message.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think splitting makes more sense as one could use the query client independently
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I split the message and checks. I made it only a warning because it might not be critical in all application (e.g SPA)
I understand but for Vue 3.3 users, not being able to use the query client within a navigation guard would be a bug. So you can also see this as a bugfix. In practice, users update multiple libraries at the same time and they should also see the requirement in peer deps. |
it should make it work in most cases yes |
26b076c
to
50b12f7
Compare
This feature requires Vue 3.3.0, which has been out for a while now. It allows using vue-query APIs in places where it is valid to use them but currently throws an error. - `hasInjectionContext()`: vuejs/core#8111 - `app.runWithContext()`: vuejs/core#7451
50b12f7
to
efb01a9
Compare
Build should be fixed with vueuse/vue-demi#241 |
I released a new version of vue-demi 🙂 |
If anyone has issue with getting this to work and comes across this PR / comment, be aware that it doesn't always work like you might think in async functions - if you I worked around this by getting the query client before my async stuff and then passing it in to my query function: const queryClient = useQueryClient();
const { isClient } = useCurrentOrg();
if (!(await isClient())) {
next('/');
return;
}
const { trial, isTrialEnded } = useTrial({}, queryClient); (where An alternative would be to just move the btw @posva, thanks for adding this! it's super useful :) |
Will this work on Nuxt plugins? I'm trying to use Edit: It works, but you must declare the |
It works but can we get rid of the warning somehow? |
This feature requires Vue 3.3.0, which has been out for a while now. It allows using vue-query APIs in places where it is valid to use them but currently throws an error.
hasInjectionContext()
: feat: hasInjectionContext() for libraries vuejs/core#8111app.runWithContext()
: feat(app): app.runWithContext() vuejs/core#7451The only thing that I think is left to do is to add extra warnings in some APIs that rely on the Reactivity API (
ref
,watch
,onScopeDispose
etc) by checking if there is a current effect scope. I think this was previously achieved by checking for a current instance within theuseQueryClient
which seems to be used in functions relying on those functions. But since it wasn't tested, I didn't add them. Let me know if I should add them.This change requires vue-demi to also be updated, if the package manager doesn't do it for their users, they shall run an
pnpm update
or similar, they can also install vue-demi and debug withpnpm why
(or similar).I can also replicate the PR for the
beta
branch