feat(signals): throw error in dev mode on state mutation

[rainerhahnekamp]

Alternative version to protected the state from mutable changes via Object.freeze.

Please check if your PR fulfills the following requirements:

• The commit message follows our guidelines: https://github.com/ngrx/platform/blob/main/CONTRIBUTING.md#commit
• Tests for the changes have been added (for bug fixes / features)
• Documentation has been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

[ ] Bugfix
[x] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Documentation content changes
[ ] Other... Please describe:

What is the current behavior?

Mutable changes in patchState don't trigger any kind of warning

Closes #4030

What is the new behavior?

In development mode (ngDevMode), Object.freeze is applied to the state, causing a runtime error on a mutable change.

Does this PR introduce a breaking change?

[x] Yes
[ ] No

BREAKING CHANGES:

The `patchState` function applies a deep freeze on the state in dev mode.

BEFORE:

const userState = signalState(initialState);
patchState(userState, (state) => {
  state.user.firstName = 'mutable change'; // mutable change which went through
  return state;
});

AFTER:

const userState = signalState(initialState);
patchState(userState, (state) => {
  state.user.firstName = 'mutable change'; // throws in dev mode
  return state;
});

[netlify]

✅ Deploy Preview for ngrx-io canceled.

Built without sensitive environment variables

Name	Link
🔨 Latest commit	bff9d2c
🔍 Latest deploy log	https://app.netlify.com/sites/ngrx-io/deploys/673a6b5605ee0600083ecb35

[samuelfernandez]

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?

[rainerhahnekamp]

@samuelfernandez

Any reasons for not returning a Readonly type

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.

[samuelfernandez]

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)

[samuelfernandez]

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? 😉

[rainerhahnekamp]

@samuelfernandez I think this is an issue for Angular. patchState is an NgRx feature, so we can improve that one. computed() and the Signal type comes from the framework itself.

[brandonroberts]

We should add a note to the docs on this behavior

[markostanimirovic]

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

[markostanimirovic]

Since this PR introduces a breaking change, we should add this to the PR description in the following format:

BREAKING CHANGES:

__description__

BEFORE:

__...__

AFTER:

__...__

Check this PR for more info: #4584

[rainerhahnekamp]

@markostanimirovic, all your requests have been incorporated:

• Breaking Changes in PR docs
• consolidation of methods in `deep-freeze.ts``
• own testing file for deep-freeze.ts
• type-renaming in freezeInDevMode()

[markostanimirovic]

@rainerhahnekamp recheck this comment: #4526 (comment)

Currently, the error will be thrown only if the state is mutated within the patchState updater. It would be good to do the full state freeze:

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'; // error

Currently, there are no errors in these cases.

[markostanimirovic]

@rainerhahnekamp It will also be useful to update documentation: #4526 (comment)

[rainerhahnekamp]

@markostanimirovic, as requested, the state itself is now frozen, not just the changes coming from patchState.
Thanks for pointing it out. This feature is became now much better.

I'll update the PR about the documentation as well.

Commits have been squashed into a "conventional commit"-friendly message.

[markostanimirovic]

LGTM 👍 Thanks Rainer!