-
-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Feature/query client on context #8599
Conversation
closing over a queryClient should now error, as we pass the queryClient directly to the queryFunctionContext
View your CI Pipeline Execution ↗ for commit 840b47f.
☁️ Nx Cloud last updated this comment at |
Sizes for commit 840b47f:
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #8599 +/- ##
==========================================
- Coverage 46.29% 46.24% -0.05%
==========================================
Files 199 199
Lines 7539 7532 -7
Branches 1724 1720 -4
==========================================
- Hits 3490 3483 -7
Misses 3670 3670
Partials 379 379 |
@@ -354,24 +354,6 @@ ruleTester.run('exhaustive-deps', rule, { | |||
} | |||
`, | |||
}, | |||
{ | |||
name: 'should ignore references of the queryClient', |
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.
note: we don’t need an exception for closing over the queryClient
anymore if we inject it into the queryFn
. @Newbie012 would you agree?
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 so. I never had to deal with more than one query client in a single app. Is it a valid use case to reference a foreign query client inside queryFn?
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.
nope, this is something the linter should definitely catch!
and try to bust it by changing sharedGlobals lol
@@ -167,6 +169,7 @@ export class Query< | |||
#initialState: QueryState<TData, TError> | |||
#revertState?: QueryState<TData, TError> | |||
#cache: QueryCache | |||
#client: QueryClient |
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.
Would be nice to get a getter for QueryClient from the instance of Query.
I missed that before for experimental persister.
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.
you probably don’t need that now that it’s passed to the context
, which the persister also has access to.
No description provided.