Skip to content
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

@ngrx/signals: Improve Developer Experience for Immutability #4030

Open
1 of 2 tasks
rainerhahnekamp opened this issue Sep 4, 2023 · 6 comments · May be fixed by #4526
Open
1 of 2 tasks

@ngrx/signals: Improve Developer Experience for Immutability #4030

rainerhahnekamp opened this issue Sep 4, 2023 · 6 comments · May be fixed by #4526
Labels
Core/Collaborators Work Work to be completed by the core team or collaborators Project: Signals

Comments

@rainerhahnekamp
Copy link
Contributor

Which @ngrx/* package(s) are relevant/related to the feature request?

store

Information

Ensuring that state updates are immutable is crucial for Signal to emit correctly and trigger necessary DOM updates, derived signals, or side effects. Currently, an immutable runtime check is available, but only in development mode due to its potential performance impact.

To enhance the robustness of applications and facilitate debugging, I propose two additional behaviours:

  • Option to enable the immutable check in production mode: There are many Angular applications where this “safety net” outweighs performance penalties by far. Especially “classic enterprise applications” containing many forms where the business logic runs in the backend.
  • $update with Readonly Types: Compilation checks are highly preferable to runtime checks because they allow potential issues to be discovered before the runtime. The $update method could be enhanced to provide the state as a recursive Readonly type to achieve this. This would extend to arrays as ReadonlyArray, removing mutable methods like push.

Example:

import { signalStore, withMethods, withState } from '@ngrx/signals';
type User = { id: number; name: string };
type UsersState = { users: User[]; loading: boolean };

declare function withImmutabilityCheck(mode: 'disabled' | 'devMode' | 'always');

const UsersStore = signalStore(
  withImmutabilityCheck('always'), // <— immutable check
  withState<UsersState>({ users: [], loading: false }),
  withMethods(({ $update, users }) => {
    return {
      addUser(user: User): void {
        $update((state) => {
          state.users.push(user); // 💥compilation error because ReadonlyArray<User>
          state.loading = true; //  💥compilation error because Readonly<UsersState>

          return state;
        });
      },
    };
  })
);

If accepted, I would be happy to provide an initial PR.

Describe any alternatives/workarounds you're currently using

No response

I would be willing to submit a PR to fix this issue

  • Yes
  • No
@samuelfernandez
Copy link
Contributor

What about tweaking signatures and return Readonly<T> on methods and similar? Good DX on compile time, no need to do it runtime, 0kb

@rainerhahnekamp
Copy link
Contributor Author

@samuelfernandez this is exactly what I am suggesting. You won't get away with a simple Readonly. It has to be nested as well. Here's the proposed type for that: https://github.com/markostanimirovic/ngrx-signal-store-playground/pull/2/files#diff-6230b858ba0b18996436aa5baa97a540a8e759a69f53b6880196d5fbf93e8f46

@jits
Copy link
Contributor

jits commented Feb 14, 2024

@rainerhahnekamp — may I suggest using this utility from ts-fest: https://github.com/sindresorhus/type-fest/blob/main/source/readonly-deep.d.ts

@rainerhahnekamp
Copy link
Contributor Author

yes, but I'd rather copy it and not have it as a dependency of ngrx itself...

@brandonroberts
Copy link
Member

That's our stance on it as well. Not everyone wants to use immer, and if they do they can provide their own wrapper around patchState. We discussed this also with the signalStore and we see it as an extension point.

See closed issue here #3998

@rainerhahnekamp
Copy link
Contributor Author

@brandonroberts, would it make sense to bring up a PR for the "type-safe"-only mode without immer or should we just close this issue?

@markostanimirovic markostanimirovic added the Core/Collaborators Work Work to be completed by the core team or collaborators label Aug 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Core/Collaborators Work Work to be completed by the core team or collaborators Project: Signals
Projects
None yet
5 participants