refactor(core): add generic utilities for resolving value-or-function patterns, replace specialized resolveStaleTime and resolveEnabled
#9212
Conversation
braden-w
changed the title
resolveStaleTime and resolveEnabled
|
| Command | Status | Duration | Result |
|---|---|---|---|
nx affected --targets=test:sherif,test:knip,tes... |
❌ Failed | 1m 11s | View ↗ |
nx run-many --target=build --exclude=examples/*... |
❌ Failed | 5s | View ↗ |
☁️ Nx Cloud last updated this comment at 2025-09-17 00:43:27 UTC
braden-w
force-pushed
the
refactor/resolveValueOrFunction
branch
from
df0249f to
2421a38
Compare
… patterns, replace specialized `resolveStaleTime` and `resolveEnabled` This commit introduces `isFunctionVariant()` and `resolveValueOrFunction()` utilities to replace repetitive `typeof === 'function'` checks throughout the codebase, consolidating the common "value or function that computes value" pattern.
braden-w
force-pushed
the
refactor/resolveValueOrFunction
branch
from
409a222 to
b28e950
Compare
… NonFunctionGuard This simplification is possible due to the introduction of generic runtime utilities that handle value-or-function resolution. The new `resolveValueOrFunction()` utility handles the distinction between direct values and functions at runtime with proper type safety, eliminating the need for complex type-level guards.
braden-w
force-pushed
the
refactor/resolveValueOrFunction
branch
from
b28e950 to
cd59cc4
Compare
braden-w
force-pushed
the
refactor/resolveValueOrFunction
branch
from
9d15984 to
427c977
Compare
| "eslint-plugin-jsdoc": "^50.5.0", | ||
| "npm-run-all2": "^5.0.0" | ||
| "npm-run-all2": "^5.0.0", | ||
| "tsup": "^8.4.0" |
There was a problem hiding this comment.
why was adding tsup necessary here? I don’t think angular uses tsup for bundling 🤔
There was a problem hiding this comment.
Removed! In earlier commits, the lack of tsup was blocking builds.
packages/query-core/src/utils.ts
Outdated
| return isFunctionVariant(value) ? value(...args) : value | ||
| } | ||
|
|
||
| export function functionalUpdate<TInput, TOutput>( |
There was a problem hiding this comment.
could we use resolveValueOrFunction here as well, as this also uses a typeof updater === 'function' check ?
packages/query-core/src/utils.ts
Outdated
| * const delay = resolveValueOrFunction(retryDelay, failureCount, error) | ||
| * ``` | ||
| */ | ||
| export function resolveValueOrFunction<T, TArgs extends Array<any>>( |
There was a problem hiding this comment.
I think a name like resolveOption would be descriptive enough
|
hm, the failing pipeline shows a lot of type-errors |
Remove specialized resolveEnabled function and consolidate functionality into the generic resolveOption utility. Both functions performed identical operations but resolveEnabled was type-specific to query enabled state. - Remove resolveEnabled function from utils.ts - Update queryObserver.ts to use resolveOption instead - Remove unused Enabled type import - Maintain identical functionality and type safety
|
it’s been two months, there’s still a failing pipeline and conflicts. please let me know if you intend to finish this PR, because it doesn’t look like it’s close to working. I’ll close it otherwise. |
|
Thanks for the ping! Will update it and get back to you by EOD. |
WalkthroughReplaced multiple per-option resolvers with a single exported resolveOption across query-core; updated query, observer, client, retryer, utils, and types to use it. Removed Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant C as Consumer
participant QO as QueryObserver
participant Q as Query
participant QC as QueryClient
participant U as utils.resolveOption
participant R as Retryer
C->>QO: setOptions(options)
QO->>U: resolveOption(options.enabled, Q)
U-->>QO: isEnabled
QO->>U: resolveOption(options.staleTime, Q)
U-->>QO: staleTime
QO->>Q: request initial/placeholder data
Q->>U: resolveOption(options.initialData, Q)
U-->>Q: initialData
alt Needs fetch
QO->>QC: fetchQuery(key, options)
QC->>U: resolveOption(options.staleTime, Q)
U-->>QC: staleTime
QC->>R: start retryer
R->>U: resolveOption(options.retryDelay, failureCount, error)
U-->>R: delay
R-->>QC: result/error
QC-->>QO: result
QO-->>C: notify result
else No fetch
QO-->>C: notify cached result
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/query-core/src/types.ts (1)
169-177: PlaceholderDataFunction type is now internal but still referenced in public APIThe
PlaceholderDataFunctiontype has been changed from exported to internal (noexportkeyword), but it's still being used in the publicQueryObserverOptionsinterface at lines 424-426. This will cause TypeScript compilation errors for consumers who need to understand or reference this type.Apply this diff to fix the type visibility issue:
-type PlaceholderDataFunction< +export type PlaceholderDataFunction< TQueryFnData = unknown, TError = DefaultError, TQueryData = TQueryFnData, TQueryKey extends QueryKey = QueryKey, > = ( previousData: TQueryData | undefined, previousQuery: Query<TQueryFnData, TError, TQueryData, TQueryKey> | undefined, ) => TQueryData | undefinedAlternatively, if the intent is to keep it internal, consider inlining the function signature directly in the
placeholderDataproperty definition.
♻️ Duplicate comments (1)
packages/query-core/src/utils.ts (1)
168-175: Use resolveOption here for consistency (duplicate of earlier feedback).This is the same “value or updater function” pattern. Reusing
resolveOptionor the localisFunctionVariantkeeps the file consistent and DRY.Apply this diff:
export function functionalUpdate<TInput, TOutput>( updater: Updater<TInput, TOutput>, input: TInput, ): TOutput { - return typeof updater === 'function' - ? (updater as (_: TInput) => TOutput)(input) - : updater + return isFunctionVariant<Updater<TInput, TOutput>, [TInput]>(updater) + ? (updater as (_: TInput) => TOutput)(input) + : (updater as TOutput) }Note: This still shares the same ambiguity if
TOutputis itself a function. If you adoptNonFunctionGuardforresolveOption, consider mirroring that here for true parity.
🧹 Nitpick comments (2)
packages/query-core/src/query.ts (1)
692-698: Consider adding type safety for InitialDataFunction patternWhile the
resolveOptionutility handles the function-or-value pattern generically, the removal of theInitialDataFunctiontype means we lose some type safety. The current implementation will accept any function, not just zero-argument functions.Consider adding a type constraint to ensure
initialDataandinitialDataUpdatedAtfunctions don't accidentally accept parameters:- const data = resolveOption(options.initialData) + const data = resolveOption(options.initialData as TData | (() => TData | undefined)) const hasData = data !== undefined const initialDataUpdatedAt = hasData - ? resolveOption(options.initialDataUpdatedAt) + ? resolveOption(options.initialDataUpdatedAt as number | (() => number | undefined) | undefined) : 0packages/query-core/src/utils.ts (1)
80-116: Tighten the type guard: return a function typed to T, not any.Returning
anyfrom the predicate unnecessarily weakens type inference in downstream callers (includingresolveOption). Have the predicate assert(...args: TArgs) => Tand preferunknown[]overArray<any>.Apply this diff:
-function isFunctionVariant<T, TArgs extends Array<any> = []>( - value: T | ((...args: TArgs) => any), -): value is (...args: TArgs) => any { +function isFunctionVariant<T, TArgs extends unknown[] = []>( + value: T | ((...args: TArgs) => T), +): value is (...args: TArgs) => T { return typeof value === 'function' }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (6)
packages/query-core/src/query.ts(4 hunks)packages/query-core/src/queryClient.ts(3 hunks)packages/query-core/src/queryObserver.ts(14 hunks)packages/query-core/src/retryer.ts(2 hunks)packages/query-core/src/types.ts(2 hunks)packages/query-core/src/utils.ts(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (5)
packages/query-core/src/queryClient.ts (2)
packages/query-core/src/queryObserver.ts (2)
query(680-697)resolveOption(364-368)packages/query-core/src/utils.ts (1)
resolveOption(161-166)
packages/query-core/src/retryer.ts (2)
packages/query-core/src/queryObserver.ts (1)
resolveOption(364-368)packages/query-core/src/utils.ts (1)
resolveOption(161-166)
packages/query-core/src/queryObserver.ts (1)
packages/query-core/src/utils.ts (1)
resolveOption(161-166)
packages/query-core/src/query.ts (2)
packages/query-core/src/queryObserver.ts (1)
resolveOption(364-368)packages/query-core/src/utils.ts (1)
resolveOption(161-166)
packages/query-core/src/utils.ts (1)
packages/query-core/src/queryObserver.ts (1)
resolveOption(364-368)
🔇 Additional comments (14)
packages/query-core/src/queryClient.ts (2)
158-159: LGTM! Clean replacement of resolveStaleTime with resolveOptionThe change correctly replaces the specialized
resolveStaleTimewith the genericresolveOptionutility, maintaining the same functionality while improving code consistency.
365-365: LGTM! Consistent use of resolveOptionThe replacement maintains the same behavior while using the unified
resolveOptionutility.packages/query-core/src/retryer.ts (1)
169-169: LGTM! Simplified delay resolutionThe change correctly replaces the manual conditional logic with the generic
resolveOptionutility, making the code more concise and consistent with the rest of the codebase.packages/query-core/src/query.ts (2)
257-257: LGTM! Consistent enabled option resolutionThe change correctly replaces
resolveEnabledwith the genericresolveOptionutility.
276-276: LGTM! Consistent staleTime resolutionThe change correctly replaces
resolveStaleTimewith the genericresolveOptionutility.packages/query-core/src/queryObserver.ts (7)
158-159: LGTM! Proper validation with resolveOptionThe validation correctly uses
resolveOptionto check the resolved value when enabled is a function, maintaining the same validation logic.
202-205: LGTM! Consistent comparison of resolved option valuesThe changes correctly compare the resolved values of
enabledandstaleTimeoptions usingresolveOption, ensuring accurate change detection.
216-217: LGTM! Consistent enabled option comparisonThe change maintains proper change detection for the enabled option using the unified utility.
345-345: LGTM! Consistent staleTime and refetchInterval resolutionAll three changes correctly use
resolveOptionfor computing stale timeout and refetch interval values.Also applies to: 366-366, 377-377
485-489: LGTM! Clean placeholder data resolutionThe change correctly uses
resolveOptionto handle both direct values and functions for placeholder data, properly passing the previous data and query as arguments when needed.
568-568: LGTM! Consistent option resolution in result creationBoth changes correctly use
resolveOptionforisEnabledflag andnotifyOnChangePropsresolution.Also applies to: 652-652
730-730: LGTM! Comprehensive update of all conditional checksAll the helper functions have been consistently updated to use
resolveOptionfor enabled and staleTime checks, maintaining the correct behavior while using the unified utility.Also applies to: 755-758, 773-773, 784-785
packages/query-core/src/utils.ts (2)
1-2: LGTM: type-only imports are correct and colocated.The new type imports for Mutation, Query, and FetchOptions are valid here and used below. No issues.
117-160: Docs/readability: solid API docs and examples.Clear, actionable, and covers both zero-arg and variadic cases. Nice.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
packages/query-core/src/utils.ts (2)
172-172: Addressed prior note: functionalUpdate now uses resolveOption.This replaces the typeof check with the new utility as requested in earlier review comments.
117-166: Critical: resolveOption will eagerly invoke function-typed values when T itself is a function (ambiguous semantics).If T is a function type, direct function “values” will be treated as the callback variant and invoked. This is the ambiguity that the former NonFunctionGuard prevented and can lead to runtime bugs and surprising side-effects. This also aligns with prior review feedback.
Fix by constraining T to non-function values for the direct-value branch and defaulting TArgs to unknown[] to improve inference:
-export function resolveOption<T, TArgs extends Array<any>>( - value: T | ((...args: TArgs) => T), - ...args: TArgs -): T { - return isFunctionVariant(value) ? value(...args) : value -} +export function resolveOption<T, TArgs extends unknown[] = []>( + value: NonFunctionGuard<T> | ((...args: TArgs) => T), + ...args: TArgs +): T { + return typeof value === 'function' + ? (value as (...args: TArgs) => T)(...args) + : (value as T) +}Also add the missing utility type and import:
- In types.ts (exported somewhere central):
// Add this near existing utility types export type NonFunctionGuard<T> = T extends Function ? never : T
- Update the import in this file to pull it in (adjust your existing types import):
import type { DefaultError, FetchStatus, MutationKey, MutationStatus, QueryFunction, QueryKey, QueryOptions, + NonFunctionGuard, } from './types'If you need an escape hatch for cases where function-shaped T must be allowed as a direct value, introduce a second API, e.g. resolveOptionAllowFunction, and use it only where intended.
Verification script (locates call sites and checks for the guard’s presence):
#!/bin/bash # 1) Confirm the NonFunctionGuard type exists (or not). rg -nP --type=ts '\btype\s+NonFunctionGuard\b|export\s+type\s+NonFunctionGuard\b' -C2 # 2) List all resolveOption call sites with context to audit ambiguous usages. rg -nP --type=ts '\bresolveOption\s*\(' -C3 # 3) Heuristic: show spots where the resolved T could plausibly be a function-typed value # (look for option names that might carry function-shaped data). rg -nP --type=ts -C2 '(initialData|placeholderData|notifyOnChangeProps|refetchInterval|retryDelay)\b'
🧹 Nitpick comments (1)
packages/query-core/src/utils.ts (1)
80-116: Prefer unknown[] over Array and avoid leaking any from the guard.Tighten the generics to unknown[] and use unknown instead of any for the guard’s return type. This improves type safety without changing behavior.
Apply:
-function isFunctionVariant<T, TArgs extends Array<any> = []>( - value: T | ((...args: TArgs) => any), -): value is (...args: TArgs) => any { +function isFunctionVariant<T, TArgs extends unknown[] = []>( + value: T | ((...args: TArgs) => unknown), +): value is (...args: TArgs) => unknown { return typeof value === 'function' }Optional: if you foresee external usage for narrowing, consider exporting this guard. Otherwise keeping it internal is fine.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
packages/query-core/src/utils.ts(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
packages/query-core/src/utils.ts (1)
packages/query-core/src/queryObserver.ts (1)
resolveOption(364-368)
🔇 Additional comments (1)
packages/query-core/src/utils.ts (1)
1-2: Type-only imports for Mutation/FetchOptions/Query — good move.This prevents runtime cycles and keeps bundles clean. No concerns.
Add NonFunction type constraint to resolveOption's generic parameter T to prevent ambiguity when T itself is a function type. This ensures the value-or-function pattern remains unambiguous by restricting T to primitives, objects, and other non-function types. - Define NonFunction union type covering all non-function types - Apply constraint to resolveOption<T extends NonFunction, ...> - Update functionalUpdate to use same constraint - Remove isFunctionVariant helper (inlined typeof check) - Simplify resolveOption implementation This prevents TypeScript errors where function types could be ambiguous in the T | (() => T) pattern, making the API more type-safe.
…r/resolveValueOrFunction
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/query-core/src/query.ts (1)
719-731: Guard against accidental invocation when initial data is itself a function value.
With the currentresolveOptionimplementation (see utils.ts), function-typed data values (e.g., storing a function asinitialData) would be invoked instead of used as data. This can cause runtime surprises and side effects.Please apply the
resolveOptionfix proposed in utils.ts (introduce a properNonFunctionGuard) to ensure we never call function-shaped data by accident. No code changes needed here once the utility is corrected.
♻️ Duplicate comments (1)
packages/query-core/src/utils.ts (1)
125-140: Critical: NonFunction type includes functions; resolver may eagerly invoke function-shaped data.
The aliastype NonFunction = /* … */ | objectdoes not exclude functions because
objectincludes functions in TypeScript. As a result,resolveOptioncan still treat function-typed data as the callback variant and invoke it. This reintroduces the ambiguity thatNonFunctionGuardpreviously prevented. This impacts options likeinitialData,placeholderData, and any future places where T could be a function value.Apply this diff to correctly guard and fix the resolver typing:
-type NonFunction = - | string - | number - | boolean - | bigint - | symbol - | null - | undefined - | object - -export function resolveOption<T extends NonFunction, TArgs extends Array<any>>( - value: T | ((...args: TArgs) => T), - ...args: TArgs -): T { - return typeof value === 'function' ? value(...args) : value -} +// Exclude functions from T so that direct function-shaped data isn't invoked. +type NonFunctionGuard<T> = T extends (...args: any[]) => any ? never : T + +export function resolveOption<T, TArgs extends unknown[] = []>( + value: NonFunctionGuard<T> | ((...args: TArgs) => T), + ...args: TArgs +): T { + return typeof value === 'function' + ? (value as (...args: TArgs) => T)(...args) + : (value as T) +}If you prefer centralizing utility types, export
NonFunctionGuardfromtypes.tsand import it here instead of declaring it locally.
🧹 Nitpick comments (3)
packages/query-core/src/utils.ts (3)
81-124: Docs: good addition, but the staleTime example is misleading.
staleTimeexpects a duration (ms), not a timestamp. Examplequery.state.dataUpdatedAt + 5000suggests returning an absolute time. Prefer a duration-based example.Suggested doc tweak (example only):
-const staleTime: number | ((query: Query) => number) = (query) => query.state.dataUpdatedAt + 5000 +const staleTime: number | ((query: Query) => number) = + (query) => (query.state.data ? 5000 : 0)
149-155: Minor typing improvement.
Preferunknown[]overArray<any>and defaultTArgsto[](already in the resolver fix). Keeps inference safer and avoidsanyleakage.This is covered by the proposed resolver diff.
476-486: Consistent boolean-or-function pattern.
shouldThrowErrorintentionally keeps a bespoke resolver; optional to convert toresolveOptionlater for consistency.If you want uniformity, you can replace the function branch with
resolveOption(throwOnError, ...params).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
packages/query-core/src/query.ts(5 hunks)packages/query-core/src/queryClient.ts(3 hunks)packages/query-core/src/queryObserver.ts(14 hunks)packages/query-core/src/types.ts(2 hunks)packages/query-core/src/utils.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/query-core/src/queryClient.ts
- packages/query-core/src/queryObserver.ts
- packages/query-core/src/types.ts
🧰 Additional context used
🧬 Code graph analysis (2)
packages/query-core/src/query.ts (2)
packages/query-core/src/utils.ts (1)
resolveOption(135-140)packages/query-core/src/queryObserver.ts (1)
resolveOption(372-376)
packages/query-core/src/utils.ts (1)
packages/query-core/src/queryObserver.ts (1)
resolveOption(372-376)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Test
🔇 Additional comments (9)
packages/query-core/src/query.ts (4)
5-5: Consolidated resolver import looks good.
ImportingresolveOptionhere aligns with the PR’s goal to unify “value-or-function” handling.
269-271: Enabled resolver: behavior preserved.
resolveOption(observer.options.enabled, this) !== falsekeeps the default “enabled unless explicitly false” semantics while allowing a future function variant.If
enabledis not yet typed to accept a function, confirm no public typings expose the function variant in this PR to avoid breaking changes.
288-289: Static stale-time check reads clearly.
UsingresolveOption(observer.options.staleTime, this) === 'static'is concise and correct for dynamic stale-time evaluators.
391-395: Nit: comment typo fix.
“retryer” spelling correction is fine.packages/query-core/src/utils.ts (5)
2-3: Type-only imports for cross-module utils look fine.
Brings the necessary types without affecting runtime.
399-422: Structural sharing path retains previous behavior.
No issues spotted; error message is helpful. Keep as-is.
153-155: Guard against Infinity already present.
isValidTimeoutcorrectly excludes negative and infinite values.
353-356: Plain array heuristic unchanged.
No change requested; just noting this intentionally excludes sparse arrays.
440-456: skipToken and ensureQueryFn: solid diagnostics.
Good developer ergonomics with helpful error messages.
| export function functionalUpdate<TInput, TOutput extends NonFunction>( | ||
| updater: Updater<TInput, TOutput>, | ||
| input: TInput, | ||
| ): TOutput { | ||
| return typeof updater === 'function' | ||
| ? (updater as (_: TInput) => TOutput)(input) | ||
| : updater | ||
| return resolveOption(updater, input) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Align functionalUpdate with the corrected resolver guard.
Ensure we don’t allow function-shaped TOutput as a direct value for updates.
Apply this diff:
-export function functionalUpdate<TInput, TOutput extends NonFunction>(
- updater: Updater<TInput, TOutput>,
- input: TInput,
-): TOutput {
- return resolveOption(updater, input)
-}
+export function functionalUpdate<TInput, TOutput>(
+ updater: Updater<TInput, NonFunctionGuard<TOutput>>,
+ input: TInput,
+): NonFunctionGuard<TOutput> {
+ return resolveOption(updater, input)
+}Additionally (outside this hunk), consider tightening the Updater alias so call sites get better errors when attempting to set function-shaped data:
// Outside selected lines — suggested change
export type Updater<TInput, TOutput> =
| NonFunctionGuard<TOutput>
| ((input: TInput) => TOutput)🤖 Prompt for AI Agents
In packages/query-core/src/utils.ts around lines 142 to 147, functionalUpdate
currently may allow a function-shaped TOutput to be returned; change it so the
updater result is guaranteed NonFunction by using the NonFunctionGuard-based
Updater alias and calling the resolver in a way that enforces that guard (i.e.,
use the NonFunctionGuard branch of Updater when resolving the option so the
returned value is typed as NonFunction), and also apply the suggested change to
the Updater type elsewhere: export type Updater<TInput, TOutput> = |
NonFunctionGuard<TOutput> | ((input: TInput) => TOutput) so call sites get
better errors when attempting to set function-shaped data.
Move NonFunction type definition above resolveOption function for better logical flow and add comprehensive JSDoc documentation explaining its purpose in preventing recursive type ambiguity in value-or-function patterns. - Add JSDoc for NonFunction type explaining constraint rationale - Move type definition before its usage for better code organization - Update resolveOption JSDoc to clarify T is constrained to non-function types - Improve code readability and developer understanding of type constraints
braden-w
force-pushed
the
refactor/resolveValueOrFunction
branch
from
64b0510 to
92a0b66
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
packages/query-core/src/utils.ts (3)
81-96: “NonFunction” still accepts functions viaobject; reintroduces function‑value ambiguityIn TypeScript,
objectincludes callable values, so this alias does not exclude functions. As a result,resolveOption<T extends NonFunction>still permitsTto be a function, and the implementation will eagerly invoke such values at runtime.Replace the alias with a conditional guard type that definitively excludes functions.
Apply this diff:
-/** - * Constraint type that excludes function types to prevent ambiguity in value-or-function patterns. - * - * This ensures that T in resolveOption<T> cannot be a function type itself, which would create - * recursive ambiguity about whether to call the function or return it as the resolved value. - */ -type NonFunction = - | string - | number - | boolean - | bigint - | symbol - | null - | undefined - | object +// Exclude function-shaped types to avoid ambiguity in value-or-function patterns +export type NonFunctionGuard<T> = T extends Function ? never : T
141-146: HardenresolveOptiongenerics: prevent invoking function‑shapedT, default args, avoidany
- Use
NonFunctionGuard<T>so direct function-shaped values are not callable.- Prefer
unknown[]overArray<any>.- Provide a default for
TArgsto improve zero-arg inference.Apply this diff:
-export function resolveOption<T extends NonFunction, TArgs extends Array<any>>( - value: T | ((...args: TArgs) => T), - ...args: TArgs -): T { - return typeof value === 'function' ? value(...args) : value -} +export function resolveOption<T, TArgs extends unknown[] = []>( + value: NonFunctionGuard<T> | ((...args: TArgs) => T), + ...args: TArgs +): T { + return typeof value === 'function' ? (value as (...args: TArgs) => T)(...args) : (value as T) +}
148-153: AlignfunctionalUpdatewith the guard so updater cannot yield function‑shaped dataThread the guard through the type parameters to ensure the result is never a function value.
Apply this diff:
-export function functionalUpdate<TInput, TOutput extends NonFunction>( - updater: Updater<TInput, TOutput>, - input: TInput, -): TOutput { - return resolveOption(updater, input) -} +export function functionalUpdate<TInput, TOutput>( + updater: Updater<TInput, NonFunctionGuard<TOutput>>, + input: TInput, +): NonFunctionGuard<TOutput> { + return resolveOption(updater, input) +}
🧹 Nitpick comments (2)
packages/query-core/src/utils.ts (2)
69-69: Optional: tightenUpdateralias to propagate the non‑function guarantee to callersThis will surface better errors at call sites trying to set function-shaped data.
Outside this hunk, consider:
-export type Updater<TInput, TOutput> = TOutput | ((input: TInput) => TOutput) +export type Updater<TInput, TOutput> = + | NonFunctionGuard<TOutput> + | ((input: TInput) => TOutput)
97-140: Nit: JSDoc example forstaleTimesuggests returning an absolute timestamp
staleTimeis a duration in ms, not an absolute time. Adjust the example to avoid confusion.Suggested snippet:
// Function with arguments (like staleTime, retryDelay) const staleTime: number | ((query: Query) => number) = (query) => query.state.data ? 5_000 : 0 const resolved = resolveOption(staleTime, query) // number
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/query-core/src/utils.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/query-core/src/utils.ts (1)
packages/query-core/src/queryObserver.ts (1)
resolveOption(372-376)
🔇 Additional comments (1)
packages/query-core/src/utils.ts (1)
2-3: Type-only imports: LGTMUsing
import typeforMutation,FetchOptions, andQueryavoids runtime costs and cycles. Looks good.
This PR introduces
isFunctionVariant()andresolveValueOrFunction()utilities to replace repetitivetypeof === 'function'checks throughout the codebase, consolidating the common "value or function that computes value" pattern.Problem and Solution
The codebase had scattered implementations of the same pattern across multiple files:
This led to code duplication, inconsistency, and maintenance overhead. We also had specialized versions like
resolveStaleTime()andresolveEnabled()that could be generalized.The new utilities provide a clean, generic solution:
While we could inline
typeof value === 'function'checks, TypeScript's type narrowing doesn't work properly with generic types in complex expressions. TheisFunctionVariant()type guard provides proper type narrowing that allowsresolveValueOrFunction()to safely call the function variant. Without it, TypeScript throws errors because it can't guarantee the type safety across the generic boundary.Both utilities support zero-argument functions (
() => T) and functions with parameters ((...args) => T), making them applicable to all value-or-function patterns in the codebase.Updated implementations in
query.ts,queryObserver.ts, andretryer.tsfor handlinginitialData,retryDelay,refetchInterval, andnotifyOnChangeProps. These utilities can replace existingresolveStaleTime()andresolveEnabled()functions in future iterations.Summary by CodeRabbit
New Features
Refactor
Chores