feat(signals): apply Object.freeze in patchState on state in dev mode
#4526
Conversation
✅ Deploy Preview for ngrx-io canceled.Built without sensitive environment variables
|
rainerhahnekamp
force-pushed
the
feat/state-freeze
branch
from
ed985d4
to
4216e54
Compare
rainerhahnekamp
changed the title
Object.freeze in patchState on state in dev modeObject.freeze in patchState on state in dev mode
rainerhahnekamp
force-pushed
the
feat/state-freeze
branch
from
4216e54
to
59bd044
Compare
|
That is great! Trying to modify the state would result in a runtime error. Any reasons for not returning a Readonly type that would catch it before on compile time? |
Yes, experience. It has shown that if you start to bend the type system too much, you might end up in some unpredictable issues. For example, if people provide some type, which could be any or maybe a very complicated type that we cannot predict, then we might run into issues where suddenly it stops working. That's the reasoning behind this decision. |
|
IMHO, I see it as complementary, don't mean type only can cover all use cases. Freezing state during development is great. But adding additional type safety can provide immediate feedback right in the IDE when writing the code. FWIW, there are simple utils that would provide deep readonly types and would cover the majority of simple cases: microsoft/TypeScript#13923 (comment) |
|
Question: I understand that, since the freeze logic is applied in the patch function, it will also work for the signal store, is that correct? If that is the case, I'd also recommend freezing the output of withComputed. In user land code I've faced the issue of trying to modify the state down in the component tree, which consumes derived state, not root state. Freezing the result of any read only signals coming from the store would be great. @eneajaho thoughts on freezedComputed for ngxtension? 😉 |
|
@samuelfernandez I think this is an issue for Angular. |
| function freezeInDevMode<State extends object>(value: State): State { | ||
| return ngDevMode ? deepFreeze(value) : value; | ||
| } | ||
|
|
There was a problem hiding this comment.
freezeInDevMode (and ngDevMode) can be moved to the deep-freeze.ts file as well. Since it's not directly related to the state object, the generic can be named T:
| function freezeInDevMode<State extends object>(value: State): State { | |
| return ngDevMode ? deepFreeze(value) : value; | |
| } | |
| function freezeInDevMode<T>(target: T): T { | |
| return ngDevMode ? deepFreeze(target) : target; | |
| } |
| describe('freezeInDevMode', () => { | ||
| it('throws on a mutable change', () => { | ||
| const userState = signalState(initialState); | ||
| expect(() => | ||
| patchState(userState, (state) => { | ||
| state.ngrx = 'mutable change'; | ||
| return state; | ||
| }) | ||
| ).toThrowError("Cannot assign to read only property 'ngrx' of object"); | ||
| }); | ||
|
|
||
| it('throws on a mutable change', () => { | ||
| const userState = signalState(initialState); | ||
| expect(() => | ||
| patchState(userState, (state) => { | ||
| state.user.firstName = 'mutable change'; | ||
| return state; | ||
| }) | ||
| ).toThrowError( | ||
| "Cannot assign to read only property 'firstName' of object" | ||
| ); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
I suggest moving these tests to deep-freeze.spec.ts and checking the behavior for both - signalState and signalStore. Take a look at these tests for inspiration: https://github.com/ngrx/platform/blob/main/modules/signals/spec/state-source.spec.ts#L49
Also, in the following cases, errors should be thrown as well:
userState.user().firstName = 'mutable change 1'; // error
// ---
getState(userState).ngrx = 'mutable change 2'; // error
// ---
const s = { user: { firstName: 'M', lastName: 'S' } };
patchState(userState, s);
s.user.firstName = 'mutable change 3'; // errorCurrently, there are no errors in these cases.
|
To mention new behavior, we can update this alert with an additional sentence that an error will be thrown in dev mode on state mutations: https://github.com/ngrx/platform/blob/main/projects/ngrx.io/content/guide/signals/signal-state.md?plain=1#L59 |
|
Since this PR introduces a breaking change, we should add this to the PR description in the following format: Check this PR for more info: #4584 |
Alternative version to protected the state from mutable changes via
Object.freeze.Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
What is the current behavior?
Mutable changes in
patchStatedon't trigger any kind of warningCloses #4030
What is the new behavior?
In development mode (
ngDevMode),Object.freezeis applied to the state, causing a runtime error on a mutable change.Does this PR introduce a breaking change?
Other information