fix(angular-query): fix injectInfiniteQuery to narrow type #9653
Conversation
WalkthroughAdds state-aware type-narrowing to Angular query result types and updates implementations/tests to expose and validate narrowed types for infinite and regular queries; also adjusts a template conditional structure. No runtime API surface changes beyond internal return-value casts. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Component
participant injectFn as injectInfiniteQuery / injectQuery
participant Result as QueryResult (typed)
Component->>injectFn: call inject*Query(...)
Note right of injectFn: implementation casts runtime result\nto CreateQueryResult | DefinedCreateQueryResult\nor CreateInfiniteQueryResult
injectFn-->>Result: return typed wrapper
Component->>Result: call isSuccess() / isError() / isPending()
alt isSuccess
Note right of Result: Type narrows to success-shaped\nCreateNarrowQueryResult → data() is defined
else isError
Note right of Result: Type narrows to error-shaped\nCreateNarrowQueryResult → error() is defined
else isPending
Note right of Result: Type narrows to pending-shaped\nCreateNarrowQueryResult
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
mdm317
changed the title
![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
🧹 Nitpick comments (3)
packages/angular-query-experimental/src/types.ts (1)
160-161: Style: spacing/formatting nitsMissing space after
=and inconsistent indentation compared to neighboring aliases. Run the formatter or align manually (also covered by the diff above).packages/angular-query-experimental/src/__tests__/inject-infinite-query.test-d.ts (2)
26-77: Good coverage for narrowing without initialDataThe three cases correctly validate narrowing behavior: data undefined on
isPending, data defined onisSuccess, and error defined onisError.Consider adding one negative assertion outside guards to ensure
data()isInfiniteData<string, unknown> | undefinedby default (no initialData).
79-141: Excellent: with initialData, pending still yields defined dataThese tests capture the critical regression vector:
isPendingshould still expose defineddata()wheninitialDatais present. The addedisSuccess/isErrorchecks round it out nicely.Optionally assert that
data()remains defined in theisErrorbranch withinitialData, mirroring runtime behavior.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/angular-query-experimental/src/__tests__/inject-infinite-query.test-d.ts(1 hunks)packages/angular-query-experimental/src/types.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
packages/angular-query-experimental/src/__tests__/inject-infinite-query.test-d.ts (1)
packages/query-core/src/types.ts (1)
InfiniteData(204-207)
packages/angular-query-experimental/src/types.ts (1)
packages/angular-query-experimental/src/signal-proxy.ts (1)
MapToSignals(4-6)
🔇 Additional comments (1)
packages/angular-query-experimental/src/types.ts (1)
160-161: Sanity-check complete — omission of BaseQueryNarrowing flags is correct
packages/query-core/src/types.ts defines isError/isPending/isSuccess; packages/angular-query-experimental/src/types.ts uses MapToSignals<OmitKeyof<TState, keyof BaseQueryNarrowing, 'safely'>> to drop those keys — no change required.
|
View your CI Pipeline Execution ↗ for commit 29a7076
☁️ Nx Cloud last updated this comment at |
| InfiniteQueryObserver as typeof QueryObserver, | ||
| ), | ||
| ) | ||
| ) as unknown as CreateInfiniteQueryResult |
There was a problem hiding this comment.
the type assertion is necessary for ts 5.4 and lower
| return runInInjectionContext(options?.injector ?? inject(Injector), () => | ||
| createBaseQuery(injectQueryFn, QueryObserver), | ||
| ) as unknown as CreateQueryResult | ||
| ) as unknown as CreateQueryResult | DefinedCreateQueryResult |
There was a problem hiding this comment.
the type assertion is necessary for ts 5.4 and lower
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/angular-query-experimental/src/inject-query.ts (1)
225-225: Consider documenting the union return type for clarity.The widened return type to
CreateQueryResult | DefinedCreateQueryResultcorrectly supports both query result variants. Similar to inject-infinite-query.ts, consider adding a comment explaining the type cast requirement.- ) as unknown as CreateQueryResult | DefinedCreateQueryResult + // Union return type supports both defined and undefined initial data scenarios + // Double cast through unknown required for TypeScript 5.4 and lower compatibility + ) as unknown as CreateQueryResult | DefinedCreateQueryResultpackages/angular-query-experimental/src/inject-infinite-query.ts (1)
124-124: Document the double-cast for TypeScript ≤ 5.4 compatibilityAdd this comment immediately above the double-cast in both files:
// Double cast through unknown required for TypeScript 5.4 and lower compatibility
Locations:
- packages/angular-query-experimental/src/inject-query.ts — at the return cast to CreateQueryResult | DefinedCreateQueryResult
- packages/angular-query-experimental/src/inject-infinite-query.ts — at the return cast to CreateInfiniteQueryResult
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
examples/angular/infinite-query-with-max-pages/src/app/components/example.component.html(1 hunks)packages/angular-query-experimental/src/__tests__/inject-query.test-d.ts(1 hunks)packages/angular-query-experimental/src/inject-infinite-query.ts(1 hunks)packages/angular-query-experimental/src/inject-query.ts(1 hunks)packages/angular-query-experimental/src/types.ts(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
packages/angular-query-experimental/src/inject-infinite-query.ts (1)
packages/angular-query-experimental/src/types.ts (1)
CreateInfiniteQueryResult(133-138)
packages/angular-query-experimental/src/types.ts (2)
packages/query-core/src/types.ts (5)
QueryObserverResult(899-904)DefaultError(47-51)OmitKeyof(19-29)InfiniteQueryObserverResult(1060-1068)DefinedInfiniteQueryObserverResult(1053-1058)packages/angular-query-experimental/src/signal-proxy.ts (1)
MapToSignals(4-6)
🔇 Additional comments (8)
packages/angular-query-experimental/src/types.ts (6)
51-56: LGTM! Correct approach for type narrowing support.Adding the generic
TStateparameter toCreateStatusBasedQueryResultenables proper type narrowing for both regular queries and infinite queries by allowing union type distribution.
58-65: LGTM! Well-structured narrowed result type.The new
CreateNarrowQueryResulttype appropriately combinesBaseQueryNarrowingwith the narrowed state signals, usingOmitKeyofwith 'safely' to prevent key collisions between the function guards and signal properties.
66-95: LGTM! Comprehensive type narrowing implementation.The updated
BaseQueryNarrowinginterface correctly implements the type narrowing pattern by:
- Making it generic over
TStateto work with different query result types- Using
CreateNarrowQueryResultwith narrowed state in the type guards- Properly extracting status-specific types with
CreateStatusBasedQueryResultThis enables the desired behavior where checking
isSuccess()narrows the data type to non-undefined.
114-120: LGTM! Consistent update to base query result type.The addition of the
TStateparameter and updated type composition alignsCreateBaseQueryResultwith the new narrowing system while maintaining backward compatibility through the default parameter.
126-132: LGTM! Proper handling of defined query results.The
DefinedCreateQueryResulttype correctly usesDefinedQueryObserverResultas the default state, ensuring type safety for queries with guaranteed data.
133-146: LGTM! Infinite query types properly support narrowing.Both
CreateInfiniteQueryResultandDefinedCreateInfiniteQueryResultare correctly updated with:
- Appropriate
TStateparameters defaulting to their respective observer result types- Consistent use of
BaseQueryNarrowingandMapToSignalswithOmitKeyof- Proper resolution of the key collision issue mentioned in past reviews
packages/angular-query-experimental/src/__tests__/inject-query.test-d.ts (1)
108-117: LGTM! Comprehensive test for error state narrowing.The test correctly verifies that when
isError()returns true, theerrorproperty is properly typed asSignal<Error>even wheninitialDatais provided. This confirms the type narrowing works as expected.examples/angular/infinite-query-with-max-pages/src/app/components/example.component.html (1)
5-11: LGTM! Template structure aligns with enhanced type guards.The change from nested if-else to independent @if blocks is a good improvement that:
- Makes each state check independent and more maintainable
- Aligns well with the type narrowing implementation where each guard (isPending, isError, isSuccess) provides specific type information
- Improves readability by clearly separating the three distinct states
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #9653 +/- ##
===========================================
+ Coverage 46.19% 94.13% +47.93%
===========================================
Files 213 21 -192
Lines 8453 426 -8027
Branches 1909 99 -1810
===========================================
- Hits 3905 401 -3504
+ Misses 4105 24 -4081
+ Partials 443 1 -442
🚀 New features to boost your workflow:
|
|
Cause
When using
Omit, a union type is transformed into an object.This causes
BaseQueryNarrowingto behave unexpectedly and fail to narrow types correctly.I believe this comment is related to the similar issue.
Problem
Using
OmitCurrently,
BaseQueryNarrowingonly works ininjectQuerywheninitialDatais not present.Repro example.
Without
OmitThe narrowing works (see: #9016),
but then
isSuccessbecomes a strange intersection type:As coderrabbitai pointed out, this can cause potential issues.
Using
DistributiveOmitAs suggested in a previous comment, using DistributiveOmit resolves the narrowing issue.
However, it changes the inferred type of data as follows:
I am not very familiar with Angular conventions, so I am not sure whether
Signal<{ wow: boolean }> | Signal<undefined>is considered valid.This Pr
Fix
BaseQueryNarrowingto narrow based on the data provided at the call siteI think we can use
DistributiveOmitifSignal<{ wow: boolean }> | Signal<undefined>is considered an acceptable type inAngular, without changing the existingBaseQueryNarrowingbehavior.Let me know your thoughts.
Summary by CodeRabbit
New Features
Examples
Tests
Refactor