refactor(angular-query): change query functions to accept an options object instead of an injector #8776
Conversation
…object instead of an injector change query functions to accept an options object instead of an injector so that it matches angular APIs such as effect and allows adding more options in the future. BREAKING CHANGE: The injector option has been replaced with an options object. The injector can now be provided with the options object. Closes TanStack#8711
dominicbachmann
force-pushed
the
enhancement/angular-consistent-injector-options
branch
from
c5074bd to
5e63c55
Compare
|
View your CI Pipeline Execution ↗ for commit 35647f7.
☁️ Nx Cloud last updated this comment at |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #8776 +/- ##
===========================================
+ Coverage 46.39% 88.65% +42.26%
===========================================
Files 199 16 -183
Lines 7551 282 -7269
Branches 1732 44 -1688
===========================================
- Hits 3503 250 -3253
+ Misses 3668 31 -3637
+ Partials 380 1 -379
🚀 New features to boost your workflow:
|
| @@ -313,3 +313,7 @@ export type CreateMutationResult< | |||
| * @public | |||
| */ | |||
| export type NonUndefinedGuard<T> = T extends undefined ? never : T | |||
|
|
|||
| export type WithOptionalInjector = { | |||
There was a problem hiding this comment.
It may seem redundant or not DRY but please give each function its own options type. These should be part of the public API. This reduces the risk of breaking changes, even if only on type level.
There was a problem hiding this comment.
Would you put the seperate option types also in the types.ts file?
There was a problem hiding this comment.
No, in the same file as the function.
| @@ -17,7 +18,7 @@ import type { InjectOptions } from '@angular/core' | |||
| * ``` | |||
| */ | |||
| export function injectQueryClient( | |||
| injectOptions: InjectOptions & { injector?: Injector } = {}, | |||
| injectOptions: InjectOptions & WithOptionalInjector = {}, | |||
There was a problem hiding this comment.
No need to change injectQueryClient as it's deprecated and will be removed.
| @@ -26,19 +27,19 @@ import type { DevtoolsErrorType } from '@tanstack/query-devtools' | |||
| * | |||
| * Consider `withDevtools` instead if you don't need this. | |||
| * @param optionsFn - A function that returns devtools panel options. | |||
| * @param injector - The Angular injector to use. | |||
| * @param options - Additional configuration | |||
There was a problem hiding this comment.
Let's think of a good name for this parameter - it's confusing if there's an optionsFn parameter and an options parameter.
There was a problem hiding this comment.
I called it options because the parameter is also called options in Angular. I also didn't like having a param called optionsFn and options. Would you change it everywhere or just here? What do you think about staticOptions or something in that direction?
There was a problem hiding this comment.
You're right. Maybe we can follow effect from Angular's lead:
export function effect(
effectFn: (onCleanup: EffectCleanupRegisterFn) => void,
options?: CreateEffectOptions,So first parameter e.g. named injectQueryFn, second options. What do you think?
change query functions to accept an options object instead of an injector so that it matches angular APIs such as effect and allows adding more options in the future.
BREAKING CHANGE: The injector option has been replaced with an options object. The injector can now be provided with the options object. Closes #8711