-
-
Notifications
You must be signed in to change notification settings - Fork 2k
feat(signals): allow user-defined signals in withState
and signalState
by splitting STATE_SOURCE
#4795
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(signals): allow user-defined signals in withState
and signalState
by splitting STATE_SOURCE
#4795
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. |
3ee3f3b
to
4f6c28b
Compare
STATE_SOURCE
STATE_SOURCE
4f6c28b
to
5d23f0c
Compare
@rainerhahnekamp It's also necessary to write |
Got it: Plural and then a line break. It looks like my IDE messed up the subject a little bit. Will check that one as well. |
1057bd7
to
be6190f
Compare
@markostanimirovic, I've updated the code or - where applicable - answered your comments. Please check, once you have time. I've also fixed a bug in af974f9. I see commits from main have been merged. Can I rebase them instead or does GitHub do that automatically meanwhile? |
You can sync changes from Btw, lint is also failing. |
574c406
to
8fd5c51
Compare
STATE_SOURCE
withState
and signalState
by splitting STATE_SOURCE
26c18a2
to
5f159a1
Compare
@markostanimirovic, @timdeschryver
I will push the PR for |
ec120da
to
3b86461
Compare
…State` BREAKING CHANGES: `withState` and `signalState` now support user-defined signals like `linkedSignal`, `resource.value`, or any other `WritableSignal`. For example: ```ts const user = signal({ id: 1, name: 'John Doe' }); const userClone = linkedSignal(user); const userValue = resource({ loader: () => Promise.resolve('user'), defaultValue: '' }); const Store = signalStore( withState({ user, userClone, userValue: userValue.value }) ); ``` The state slices don't change: ```ts store.user; // DeepSignal<{ id: number, name: string }> store.userClone; // DeepSignal<{ id: number, name: string }> store.userValue; // Signal<string> ``` The behavior of `linkedSignal` and `resource` is preserved. Since the SignalStore no longer creates the signals internally in these cases, signals passed into `withState` can also be changed externally. This is a foundational change to enable features like `withLinkedState` and potential support for `withResource`. The internal `STATE_SOURCE` is no longer represented as a single `WritableSignal` holding the entire state object. Instead, each top-level property becomes its own `WritableSignal`, or remains as-is if the user already provides a `WritableSignal`. ## Motivation - Internal creation of signals limited flexibility; users couldn’t bring their own signals into the store - Reusing existing signals enables future features like `withLinkedState` or `withResource`. - Splitting state into per-key signals improves the performance, because the root is not the complete state anymore. ## Change to `STATE_SOURCE` Given: ```ts type User = { firstname: string; lastname: string; }; ``` ### Before ```ts STATE_SOURCE: WritableSignal<User>; ``` ### Now ```ts STATE_SOURCE: { firstname: WritableSignal<string>; lastname: WritableSignal<string>; }; ``` ## Breaking Changes ### 1. Different object reference The returned object from `signalState()` or `getState()` no longer keeps the same object identity: ```ts const obj = { ngrx: 'rocks' }; const state = signalState(obj); ``` **Before:** ```ts state() === obj; // ✅ true ``` **Now:** ```ts state() === obj; // ❌ false ``` --- ### 2. No signal change on empty patch Empty patches no longer emit updates, since no signal is mutated: ```ts const state = signalState({ ngrx: 'rocks' }); let count = 0; effect(() => count++); TestBed.flushEffects(); expect(count).toBe(1); patchState(state, {}); ``` **Before:** ```ts expect(count).toBe(2); // triggered ``` **Now:** ```ts expect(count).toBe(1); // no update ``` --- ### 3. No wrapping of top-level `WritableSignal`s ```ts const Store = signalStore( withState({ foo: signal('bar') }) ); const store = new Store(); ``` **Before:** ```ts store.foo; // Signal<Signal<string>> ``` **Now:** ```ts store.foo; // Signal<string> ``` --- ### 4.: `patchState` no longer supports `Record` as root state Using a `Record`as the root state is no longer supported by `patchState`. **Before:** ```ts const Store = signalStore( { providedIn: 'root' }, withState<Record<number, number>>({}), withMethods((store) => ({ addNumber(num: number): void { patchState(store, { [num]: num, }); }, })) ); store.addNumber(1); store.addNumber(2); expect(getState(store)).toEqual({ 1: 1, 2: 2 }); ``` **After:** ```ts const Store = signalStore( { providedIn: 'root' }, withState<Record<number, number>>({}), withMethods((store) => ({ addNumber(num: number): void { patchState(store, { [num]: num, }); }, })) ); store.addNumber(1); store.addNumber(2); expect(getState(store)).toEqual({}); // ❌ Nothing updated ``` If dynamic keys are needed, consider managing them inside a nested signal instead. ## Further Changes - `signalStoreFeature` updated due to changes in `WritableStateSource` - `patchState` now uses `NoInfer` on `updaters` to prevent incorrect type inference when chaining
Co-authored-by: michael-small <33669563+michael-small@users.noreply.github.com>
Co-authored-by: Tim Deschryver <28659384+timdeschryver@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work Rainer!
Please check this comment: #4795 (comment)
It's necessary to add "BREAKING CHANGES: ..." at the end of the issue description in a plain text format. This will be copied in a commit message body.
withState
and signalState
by splitting STATE_SOURCE
withState
and signalState
by splitting STATE_SOURCE
Co-authored-by: Marko Stanimirović <markostanimirovic95@gmail.com>
Co-authored-by: Marko Stanimirović <markostanimirovic95@gmail.com>
Co-authored-by: Marko Stanimirović <markostanimirovic95@gmail.com>
Co-authored-by: Marko Stanimirović <markostanimirovic95@gmail.com>
Co-authored-by: Marko Stanimirović <markostanimirovic95@gmail.com>
Co-authored-by: Marko Stanimirović <markostanimirovic95@gmail.com>
@markostanimirovic: I think we can start a new review round. There weren't actually that many changes:
|
BREAKING CHANGES:
withState
andsignalState
now support user-defined signals likelinkedSignal
,resource.value
, or any otherWritableSignal
.For example:
The state slices don't change:
The behavior of
linkedSignal
andresource
is preserved. Since theSignalStore no longer creates the signals internally in these cases,
signals passed into
withState
can also be changed externally.This is a foundational change to enable features like
withLinkedState
and potential support for
withResource
.The internal
STATE_SOURCE
is no longer represented as a singleWritableSignal
holding the entire state object. Instead, each top-levelproperty becomes its own
WritableSignal
, or remains as-is if the useralready provides a
WritableSignal
.Motivation
their own signals into the store
withLinkedState
or
withResource
.the root is not the complete state anymore.
Change to
STATE_SOURCE
Given:
Before
Now
Breaking Changes
1. Different object reference
The returned object from
signalState()
orgetState()
no longer keepsthe same object identity:
Before:
Now:
2. No signal change on empty patch
Empty patches no longer emit updates, since no signal is mutated:
Before:
Now:
3. No wrapping of top-level
WritableSignal
sBefore:
Now:
4.:
patchState
no longer supportsRecord
as root stateUsing a
Record
as the root state is no longer supported bypatchState
.Before:
After:
If dynamic keys are needed, consider managing them inside a nested signal instead.
Further Changes
signalStoreFeature
updated due to changes inWritableStateSource
patchState
now usesNoInfer
onupdaters
to prevent incorrect typeinference when chaining
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?
Closes #
What is the new behavior?
Does this PR introduce a breaking change?
Other information