refactor(angular-query): Remove effects from base query #9814
Conversation
🦋 Changeset detectedLatest commit: d4f6b87 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
WalkthroughRefactors the base query in @tanstack/angular-query-experimental to stop relying on effect execution, introducing signal-based persistent observer handling, a linkedResultSignal, PendingTaskRef-based fetch tracking, DestroyRef cleanup, and test updates replacing TestBed.tick with Vitest async timer APIs. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Component
participant createBaseQuery
participant Observer
participant linkedResultSignal
participant PendingTaskRef
participant DestroyRef
Component->>createBaseQuery: instantiate query
createBaseQuery->>Observer: create persistent observer (untracked updates)
createBaseQuery->>linkedResultSignal: register proactive effect
linkedResultSignal->>Observer: trigger evaluation / execution
Observer->>PendingTaskRef: add pending task (fetching)
Observer->>linkedResultSignal: update result signal
PendingTaskRef->>linkedResultSignal: clear when idle
Component->>DestroyRef: component destroyed
DestroyRef->>createBaseQuery: invoke cleanup
createBaseQuery->>Observer: unsubscribe
createBaseQuery->>PendingTaskRef: clear pending tasks
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🔇 Additional comments (1)
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 |
![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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.changeset/little-parks-arrive.md(1 hunks)packages/angular-query-experimental/src/__tests__/inject-query.test.ts(3 hunks)packages/angular-query-experimental/src/create-base-query.ts(6 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/angular-query-experimental/src/create-base-query.ts (4)
packages/angular-query-experimental/src/pending-tasks-compat.ts (2)
PENDING_TASKS(9-28)PendingTaskRef(7-7)packages/angular-query-experimental/src/inject-is-restoring.ts (1)
injectIsRestoring(32-36)packages/query-core/src/utils.ts (1)
noop(82-82)packages/angular-query-experimental/src/signal-proxy.ts (1)
signalProxy(14-46)
🔇 Additional comments (9)
packages/angular-query-experimental/src/create-base-query.ts (6)
44-48: LGTM: Proper lifecycle management setup.The introduction of
DestroyRefand the switch toisRestoringSignal()correctly establishes reactive cleanup and restoration state handling.
64-84: LGTM: Efficient persistent observer pattern.The persistent observer with reactive option updates is a solid improvement. The use of
untracked()correctly prevents infinite reactive loops while keeping options synchronized.
97-155: LGTM: Well-implemented signal-based result tracking.The
linkedResultSignalpattern correctly:
- Manages subscription lifecycle with proper cleanup
- Handles pending task tracking at both initialization and during state changes
- Runs subscriptions outside Angular zone for performance
- Propagates errors through
ngZone.onError.emitbefore throwingThe dual pending task checks (lines 109-111 and 120-127) correctly handle both immediate fetching state and subsequent transitions.
157-157: LGTM: Proper cleanup on destroy.The
destroyRef.onDestroyhook ensures subscriptions and pending tasks are cleaned up when the injection context is destroyed.
159-168: LGTM: Proactive query execution via effect.This effect ensures queries execute immediately rather than lazily on first read. The comment correctly notes that removing this effect would enable lazy execution, which aligns with the PR objectives mentioning a possible future option for lazy query execution.
170-186: LGTM: Correct result composition with refetch handling.The double-call pattern
linkedResultSignal()()correctly reads the writable signal (as documented in lines 93-95). The refetch method wrapper ensures options are synchronized before execution, which is essential for correct behavior when options have changed.packages/angular-query-experimental/src/__tests__/inject-query.test.ts (3)
662-677: LGTM: Correct async timer migration.The test correctly migrates from synchronous
TestBed.tick()to asynchronous Vitest timer APIs (vi.runAllTimersAsync()), with properapp.whenStable()awaits to ensure pending tasks complete before assertions.
705-718: LGTM: Appropriate use of runOnlyPendingTimersAsync.The use of
vi.runOnlyPendingTimersAsync()is more precise thanrunAllTimersAsync()for this test, as it only executes currently pending timers without triggering additional timer callbacks scheduled during execution.
743-757: LGTM: Consistent async timing in invalidation test.The test correctly awaits timer execution and stability before asserting on query state, both for initial query execution and after invalidation.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
1 participant
🎯 Changes
This PR is a follow-up to #9098 and aligns queries with the proposed change for mutations. With this change queries should have a more consistent state as they no longer depend on Angular to sync the state via effects.
Additionally, this change would allow to (optionally?) lazily execute queries in future, i.e., queries would only run if the data is read by some consumer.
✅ Checklist
pnpm run test:pr.🚀 Release Impact
Summary by CodeRabbit