Pending tasks and inject queries type experiments - #1
benjavicente wants to merge 7 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
This PR refactors the Angular Query experimental package to align with Angular v19 requirements and improve handling of pending tasks and cleanup logic. The changes focus on type system improvements for the combine callback, better lifecycle management for mutations and queries, and enhanced test stability.
Key changes:
- Introduced
CombineResultstype mapping forinjectQueriescombine callback to use coreQueryObserverResulttypes instead of Angular signal-wrapped types - Added
destroyedflag ininjectMutationto prevent state updates after component destruction - Enhanced pending task tracking in
create-base-queryto check initial query state
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
packages/angular-query-experimental/src/inject-queries.ts |
Added GetQueryObserverResultForCombine and CombineResults types to correctly type the combine callback parameter with core QueryObserverResult instead of signal-wrapped results |
packages/angular-query-experimental/src/inject-mutation.ts |
Added destroyed flag to prevent state updates from subscription callbacks after component destruction |
packages/angular-query-experimental/src/create-base-query.ts |
Added initial state check to start pending task if query is already fetching when component initializes |
packages/angular-query-experimental/src/__tests__/pending-tasks.test.ts |
Updated tests to use fixture.whenStable() instead of app.whenStable() and added stability checks |
packages/angular-query-experimental/src/__tests__/inject-query.test.ts |
Updated tests to properly wait for stability using fixture.whenStable() with timer advancement |
packages/angular-query-experimental/src/__tests__/inject-queries.test.ts |
Added test for combine functionality, updated test setup to use fake timers, scoped queryClient to beforeEach |
packages/angular-query-experimental/src/__tests__/inject-queries.test-d.ts |
Added type test verifying combine callback receives correct plain value types |
packages/angular-query-experimental/src/__tests__/inject-infinite-query.test-d.ts |
Simplified to pure type-checking test by removing unnecessary TestBed setup and runtime code |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| // Destroy component while mutation is running | ||
| fixture.destroy() | ||
| fixture.detectChanges() |
There was a problem hiding this comment.
Calling fixture.detectChanges() after fixture.destroy() is problematic and can lead to errors or unexpected behavior in Angular. Once a fixture is destroyed, change detection should not be triggered on it. This line should be removed.
| fixture.detectChanges() | |
benjavicente
left a comment
benjavicente
left a comment
There was a problem hiding this comment.
- Race conditions on pending tasks. Our tests should verify that the pending task started and ended.
- Inject queries combine type helper.
| if (destroyed) return | ||
|
|
||
| // Track pending task when mutation is pending | ||
| if (state.isPending && !taskCleanupRef) { | ||
| taskCleanupRef = pendingTasks.add() |
There was a problem hiding this comment.
Some race condition caused that the subscription to be called after the component was destroyed, starting a pending task without a callback to stop it.
| type GetQueryObserverResultForCombine<T> = T extends { | ||
| queryFnData: any | ||
| error?: infer TError | ||
| data: infer TData | ||
| } | ||
| ? QueryObserverResult<TData, TError> | ||
| : T extends { queryFnData: infer TQueryFnData; error?: infer TError } | ||
| ? QueryObserverResult<TQueryFnData, TError> | ||
| : T extends { data: infer TData; error?: infer TError } | ||
| ? QueryObserverResult<TData, TError> | ||
| : T extends [any, infer TError, infer TData] | ||
| ? QueryObserverResult<TData, TError> | ||
| : T extends [infer TQueryFnData, infer TError] | ||
| ? QueryObserverResult<TQueryFnData, TError> | ||
| : T extends [infer TQueryFnData] | ||
| ? QueryObserverResult<TQueryFnData> | ||
| : T extends { | ||
| queryFn?: | ||
| | QueryFunction<infer TQueryFnData, any> | ||
| | SkipTokenForCreateQueries | ||
| select?: (data: any) => infer TData | ||
| throwOnError?: ThrowOnError<any, infer TError, any, any> | ||
| } | ||
| ? QueryObserverResult< | ||
| unknown extends TData ? TQueryFnData : TData, | ||
| unknown extends TError ? DefaultError : TError | ||
| > | ||
| : QueryObserverResult | ||
|
|
||
| /** | ||
| * CombineResults reducer recursively maps type param to core QueryObserverResult (for combine callback) | ||
| */ | ||
| type CombineResults< | ||
| T extends Array<any>, | ||
| TResults extends Array<any> = [], | ||
| TDepth extends ReadonlyArray<number> = [], | ||
| > = TDepth['length'] extends MAXIMUM_DEPTH | ||
| ? Array<QueryObserverResult> | ||
| : T extends [] | ||
| ? [] | ||
| : T extends [infer Head] | ||
| ? [...TResults, GetQueryObserverResultForCombine<Head>] | ||
| : T extends [infer Head, ...infer Tails] | ||
| ? CombineResults< | ||
| [...Tails], | ||
| [...TResults, GetQueryObserverResultForCombine<Head>], | ||
| [...TDepth, 1] | ||
| > | ||
| : { [K in keyof T]: GetQueryObserverResultForCombine<T[K]> } | ||
|
|
There was a problem hiding this comment.
Opus 4.5. It seems to work as expected, and it matches the structure of the type helpers. I'm not a type-wizard to understand it completely without help.
| const initialState = observer.getCurrentResult() | ||
| if (initialState.fetchStatus !== 'idle') { | ||
| startPendingTask() | ||
| } | ||
|
|
||
| return observer.subscribe((state) => { | ||
| if (state.fetchStatus !== 'idle') { | ||
| startPendingTask() | ||
| } else { | ||
| stopPendingTask() | ||
| } | ||
|
|
||
| queueMicrotask(() => { |
There was a problem hiding this comment.
If the pending task is started in queueMicrotask, it would be too late to fixture.whenStable(). It works with app.whenStable(), since the fixture checks synchronously the stable status to return early, while the app seems async.
| const stablePromise = app.whenStable() | ||
| await vi.advanceTimersByTimeAsync(10) | ||
| await stablePromise |
There was a problem hiding this comment.
The problem of awaiting app.whenStable() directly is that it could depend on time advancing, so the promise needs to be awaited after advancing time.
benjavicente
changed the title
🎯 Changes
✅ Checklist
pnpm run test:pr.🚀 Release Impact