feat(signals): allow access to methods in withComputed
#4864
Conversation
✅ Deploy Preview for ngrx-io ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify project configuration. |
✅ Deploy Preview for ngrx-site-v19 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
rainerhahnekamp
requested review from
markostanimirovic,
timdeschryver and
Copilot
There was a problem hiding this comment.
Pull Request Overview
This PR updates the withComputed feature to include store methods in its computed factory, and adds type-level tests to verify method access and state immutability.
- Extend
withComputedsignature to includeInput['methods']in the store parameter - Add type tests ensuring computed factories can access
props,statesignals, andmethods, and cannot patch state - Clean up imports in spec files for consistency
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| modules/signals/src/with-computed.ts | Include Input['methods'] in the store type for withComputed |
| modules/signals/spec/with-computed.spec.ts | Move and dedupe TestBed import |
| modules/signals/spec/types/with-computed.types.spec.ts | Refactor imports and add code snippets testing method access and immutability |
Comments suppressed due to low confidence (1)
modules/signals/spec/types/with-computed.types.spec.ts:16
- The
TestBedimport is unused in the type-level tests—removing it can keep the imports focused and clear.
import { TestBed } from '@angular/core/testing';
| @@ -1,4 +1,5 @@ | |||
| import { computed, signal } from '@angular/core'; | |||
| import { TestBed } from '@angular/core/testing'; | |||
There was a problem hiding this comment.
The TestBed import is not used in this file—consider removing it to reduce clutter.
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
we can remove TestBed import if it's unused
There was a problem hiding this comment.
We actually use it. Don't know why Copilot has here a different opinion....
There was a problem hiding this comment.
Documentation needs to be updated as well: https://ngrx.io/guide/signals/signal-store#defining-store-properties
No need to add additional examples, just update the following part: utilizing previously defined state signals and properties => utilizing previously defined state signals, properties, and methods
| @@ -1,4 +1,5 @@ | |||
| import { computed, signal } from '@angular/core'; | |||
| import { TestBed } from '@angular/core/testing'; | |||
There was a problem hiding this comment.
we can remove TestBed import if it's unused
Co-authored-by: Marko Stanimirović <markostanimirovic95@gmail.com>
|
@markostanimirovic, @timdeschryver, I think we are ready to go with that one. |
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
What is the current behavior?
withComputeddoesn't have access tomethodsCloses #4846
What is the new behavior?
Does this PR introduce a breaking change?