refactor(angular-query): injectMutationState use signal sandwich pattern #9808
Conversation
π¦ Changeset detectedLatest commit: eddedf7 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 |
WalkthroughThe Changes
Sequence Diagram(s)sequenceDiagram
participant User as User Code
participant Inject as injectMutationState()
participant Cache as mutationCache
participant Signal as linkedStateSignal
rect rgb(200, 220, 255)
Note over Inject: Initialization
Inject->>Signal: Initialize from getResult()
end
rect rgb(220, 240, 220)
Note over Cache,Signal: State Updates
Cache->>Inject: mutation event
Inject->>Signal: updateMutationState(untracked)
Signal->>Signal: replaceEqualDeep deduplicate
end
rect rgb(240, 240, 200)
Note over Inject,User: Return & Subscribe
Inject->>User: computed(linkedStateSignal()())
User->>User: Signal subscription
end
Estimated code review effortπ― 3 (Moderate) | β±οΈ ~20 minutes The refactor introduces a new architectural pattern (linked-state signal) with meaningful logic changes to state initialization, subscription registration, and derived state computation. While confined to a single file, the conceptual shift from multi-signal to unified signal-sandwich pattern and the introduction of Poem
Pre-merge checks and finishing touchesβ Failed checks (1 warning)
β Passed checks (2 passed)
β¨ Finishing touches
π§ͺ Generate unit tests (beta)
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 |
|
View your CI Pipeline Execution β for commit eddedf7
βοΈ Nx Cloud last updated this comment at |
Codecov Reportβ
All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #9808 +/- ##
=======================================
Coverage ? 94.03%
=======================================
Files ? 21
Lines ? 419
Branches ? 95
=======================================
Hits ? 394
Misses ? 24
Partials ? 1
π New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
π§Ή Nitpick comments (2)
packages/angular-query-experimental/src/inject-mutation-state.ts (2)
53-53: Improve parameter documentation.The documentation for
mutationStateFnis incomplete (ends with a dash). Consider adding a description such as:- * @param mutationStateFn - + * @param mutationStateFn - A function returning mutation state options (filters, select). Reactive: if it reads from signals, the result will reset when those signals change.This clarifies both the purpose and the reactive behavior of this parameter.
68-78: Signal sandwich pattern is correct but complex.The
computed(() => signal(...))pattern effectively implements a linked signal for Angular < 19 compatibility. The pattern correctly:
- Creates a new signal with fresh state when
mutationStateFn()dependencies change- Maintains a stable signal for updates when dependencies don't change
- Allows the subscription to update the current signal state
The comment explaining the extra parentheses is helpful, though the pattern remains non-obvious for maintainers unfamiliar with this approach.
Consider adding an example to the comment:
// Example usage: // const options = signal({ filters: { status: 'pending' } }); // const state = injectMutationState(() => options()); // // state()() returns the current array // // When options() changes, state resets to fresh derived data // // When mutations change, state updates reactively
π Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
π Files selected for processing (2)
.changeset/inject-mutation-state-signal-sandwich.md(1 hunks)packages/angular-query-experimental/src/inject-mutation-state.ts(3 hunks)
π§° Additional context used
𧬠Code graph analysis (1)
packages/angular-query-experimental/src/inject-mutation-state.ts (2)
packages/query-core/src/mutation.ts (1)
MutationState(26-41)packages/vue-query/src/useMutationState.ts (1)
MutationStateOptions(51-54)
β° 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). (2)
- GitHub Check: Preview
- GitHub Check: Test
π Additional comments (5)
.changeset/inject-mutation-state-signal-sandwich.md (1)
1-5: Consider whether this should be a minor version instead of patch.While the internal refactor is non-breaking, the parameter rename from
injectMutationStateFntomutationStateFn(line 58 in inject-mutation-state.ts) changes the public API. Although this is a positional parameter with a default value, the name change could affect:
- Users relying on parameter names for code clarity
- Auto-complete and IDE hints
- Generated documentation
Given this is an experimental package, a patch release may be acceptable, but consider if a minor version bump would better signal the API surface change.
packages/angular-query-experimental/src/inject-mutation-state.ts (4)
9-11: LGTM: Import changes align with the refactor.The addition of
untrackedand explicit import ofreplaceEqualDeepsupport the new signal sandwich pattern and deduplication logic.
80-88: Update flow is correct; consider caching implications.The
updateMutationStatefunction correctly:
- Runs inside Angular zone for change detection
- Uses
untrackedto prevent accidental dependency creation (defensive, as there's no reactive context here)- Updates the current signal with deduplicated results
Minor consideration:
mutationStateFn()is called on every mutation cache update (line 84). If this function is expensive or reads from multiple signals, consider the performance implications. Users should ensure theirmutationStateFnis lightweight and stable.The pattern assumes
mutationStateFn()is stable and cheap to call. Consider adding a comment documenting this expectation, or verify that this assumption holds in typical usage patterns.
90-94: LGTM: Subscription pattern is correct.Running the subscription outside Angular zone and explicitly entering the zone in
updateMutationStateis the correct pattern for optimal change detection. Cleanup is properly registered.
96-96: LGTM: Return value correctly unwraps the signal sandwich.The
computed(() => linkedStateSignal()())pattern correctly creates a read-only signal that updates when either:
- The outer computed re-runs (when
mutationStateFn()dependencies change)- The inner signal updates (when mutations change)
The double-parentheses syntax is unusual but correctly documented in the comment above.
arnoud-dv
force-pushed
the
refactor/inject-mutation-state-signal-sandwich
branch
from
66d80fa to
eddedf7
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
π§Ή Nitpick comments (3)
packages/angular-query-experimental/src/inject-mutation-state.ts (2)
80-89: Return a readonly array to discourage external mutation.Consumers can currently mutate the returned array, undermining structural sharing and deduplication. Prefer
ReadonlyArray<TResult>throughout and annotate the inner signal accordingly.Apply this diff:
-): Array<TResult> { +): ReadonlyArray<TResult> { return mutationCache .findAll(options.filters) .map( (mutation): TResult => (options.select ? options.select(mutation) : mutation.state) as TResult, ) } @@ -export function injectMutationState<TResult = MutationState>( +export function injectMutationState<TResult = MutationState>( mutationStateFn: () => MutationStateOptions<TResult> = () => ({}), options?: InjectMutationStateOptions, -): Signal<Array<TResult>> { +): Signal<ReadonlyArray<TResult>> { @@ - const linkedStateSignal = computed(() => - signal(getResult(mutationCache, mutationStateFn())), - ) + const linkedStateSignal = computed(() => + signal<ReadonlyArray<TResult>>(getResult(mutationCache, mutationStateFn())), + ) @@ - return computed(() => linkedStateSignal()()) + return computed<ReadonlyArray<TResult>>(() => linkedStateSignal()())Also applies to: 20-40, 57-61, 76-79, 96-96
51-59: Param rename is doc-only; consider clarifying to avoid perceived API break.Renaming the parameter to
mutationStateFnis non-breaking in TS (positional, not named args). Consider noting this in the changeset to prevent confusion for users scanning release notes..changeset/tidy-parents-send.md (1)
5-5: Clarify that the parameter rename is non-breaking.Add a sentence to state that the
injectMutationStateparameter rename is documentation-only and does not change the call signature or behavior.Proposed wording:
- βThis is a refactor only; the
injectMutationStatecall signature is unchanged. The parameter was renamed in docs for clarity; no code changes are required by consumers.β
π Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
π Files selected for processing (3)
.changeset/inject-is-mutating-readonly.md(0 hunks).changeset/tidy-parents-send.md(1 hunks)packages/angular-query-experimental/src/inject-mutation-state.ts(3 hunks)
π€ Files with no reviewable changes (1)
- .changeset/inject-is-mutating-readonly.md
π§° Additional context used
𧬠Code graph analysis (1)
packages/angular-query-experimental/src/inject-mutation-state.ts (2)
packages/query-core/src/mutation.ts (1)
MutationState(26-41)packages/vue-query/src/useMutationState.ts (1)
MutationStateOptions(51-54)
β° 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). (2)
- GitHub Check: Preview
- GitHub Check: Test
π Additional comments (2)
packages/angular-query-experimental/src/inject-mutation-state.ts (2)
68-79: Signal sandwich is correctly applied (linked-signal substitute).Creating a writable signal inside a computed and reading/writing it via
result()()/result().update(...)is a solid Angular <19-compatible linked-signal pattern. Nicely done.
90-95: Good zone boundary and teardown.Subscribing outside Angular and re-entering via
ngZone.run(...), plusdestroyRef.onDestroy(unsubscribe), prevents change-detection thrash and leaks. LGTM.
1 participant
π― Changes
β Checklist
pnpm run test:pr.π Release Impact
Summary by CodeRabbit
Release Notes
injectMutationStateparameter frominjectMutationStateFntomutationStateFnfor improved API consistency.