feat(signal): better guardrails to avoid wrong signal object mutations

[mauriziocescon]

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

core

Description

Hey!

I'm aware that the "not reactive" mutation of signal-objects is a known tradeoff (see for example #58440). Nevertheless, I wonder if there's really no way to mitigate the potential problems coming from it by throwing some sort of errors (at least at dev time).

Here is a minimal example:

@Injectable({
  providedIn: 'root',
})
export class Store {
  private readonly state = signal({ value: 0 });

  readonly counter = computed(() => this.state());
  readonly frozenCounter = computed(() => Object.freeze(this.state()));

  increase() {
    this.state.update((v) => ({ ...v, value: v.value + 1 }));
  }

  decrease() {
    this.state.update((v) => ({ ...v, value: v.value - 1 }));
  }
}

@Component({
  template: `
    <h3>App</h3>
    <p>Computed: {{ value() }}</p>
    <p>Store: {{ store.counter().value }}</p>
  `,
})
export class App {
  protected readonly store = inject(Store);
  protected readonly value = computed(() => this.store.counter().value);

  increase() {
    // this.store.increase(); // ✅ correct
    this.store.counter().value++; // ❌ wrong, but no error
    // this.store.frozenCounter().value++; // ✅ fine: throws an error
  }

  decrease() {
    // this.store.decrease(); // ✅ correct
    this.store.counter().value--; // ❌ wrong, but no error
    // this.store.frozenCounter().value--; // ✅ fine: throws an error
  }
}

https://stackblitz.com/edit/stackblitz-starters-bqez6f1y

A service acting as a store is just a popular use case, but essentially any signal having a (serialisable) object is potentially affected by the same problem. In all such cases, exposing / passing computeds or readonly signals don't make 100% sure the state cannot be accidentally mutated. At least, I'm not aware of any "ng-way" to achieve it.

So I wonder: would be freezing / deepFreezing signals something the team has considered? For example as an opt-in in dev mode... as recently done by ngrx-store team. Alternatively, any other suggestion in userland to address the problem?

I understand this might favour a more "strictly immutable approach" to the data flow... and in general, it might make the signal implementation more complicated.

I hope I haven't missed anything trivial (in case sorry).
Thanks a lot!

Proposed solution

That I'm aware of:

• Opt-in freeze / deepFreeze for all signals (at least in dev mode) + runtime errors,
• expose a read / transform / ... option for signal / computed and cherry-pick the cases to freeze: const state = signal({ value: 0 }, {read: Object.freeze}) + runtime errors,
• purely at ts level (likely impossible => breaking change + I'm for sure missing something else 😅):

// freeze case, deepFreeze likely more complicated (if DeepReadonly possible 😅)
type Readonly<T> = { readonly [P in keyof T]: T[P] };

declare function computed<T>(computation: () => T, options?: CreateComputedOptions<T>): Signal<Readonly<T>>;

[pawel-twardziak]

Hi @mauriziocescon 👋

I've proposed one of your solutions before there #58440.

We tried this, actually, and found that it doesn't really work in practice.

In principle it makes sense, but the consequence is that developers must structure all APIs and code that is receiving values from signals to accept readonly values. That is, if your value comes from Signal<User>, you can't just use it with a utility function getRoles(user: User). That function would have to be written as function getRoles(user: ReadonlyDeep<User>). This is an extremely burdensome and infectious requirement, and would severely limit the ability to start using signals in codebases not written in this style (which in our experience is all of them).

Originally posted by @alxhub in #58440

So, @alxhub this topic comes up again ➿

I recommend something likt the ngrx signals has ngrx/platform#4526 by @rainerhahnekamp by default + a local/global flag to disable that behaving in dev mode for backward compatibility of all the existing codebases.

[e-oz]

@pawel-twardziak freezing can work without Readonly types. It will just throw (that's why it is better to do it with isDevMode() only).

[pawel-twardziak]

@pawel-twardziak freezing can work without Readonly types. It will just throw (that's why it is better to do it with isDevMode() only).

Yes, that's the approach in @ngrx/signals I've mentioned :)

[pawel-twardziak]

@alxhub - does this proposal add up? :)

[natebundy]

Hi @mauriziocescon 👋

I've proposed one of your solutions before there #58440.

We tried this, actually, and found that it doesn't really work in practice.
In principle it makes sense, but the consequence is that developers must structure all APIs and code that is receiving values from signals to accept readonly values. That is, if your value comes from Signal<User>, you can't just use it with a utility function getRoles(user: User). That function would have to be written as function getRoles(user: ReadonlyDeep<User>). This is an extremely burdensome and infectious requirement, and would severely limit the ability to start using signals in codebases not written in this style (which in our experience is all of them).

Originally posted by @alxhub in #58440

So, @alxhub this topic comes up again ➿

I recommend something likt the ngrx signals has ngrx/platform#4526 by @rainerhahnekamp by default + a local/global flag to disable that behaving in dev mode for backward compatibility of all the existing codebases.

I've tried to do readonly for RxJS observables purely through TypeScript types for my project's codebase, but the problem is as soon as a function widens the type (i.e. from Readonly<T> to T) in a way that TypeScript won't catch it and throw an error at compile time, from that point on, that type is no longer the Readonly type, and somebody somewhere will inevitably mutate that type. (Just today, in fact, I'm dealing with a mutation that's been in our code base for 4 years and we've just now caught it.) I've considered going back and adding an Object.freeze, but then you have to remember to re-freeze in every single function where you clone as well, which is onerous.

I would absolutely love Signals to do something like Object.freeze in dev mode. In my ideal world, Signals would return deep Readonly types and force specifying readonly types for function parameters - it's nice to have the compile time check that warns you that something's off, forcing you to clone to a new object or deep clone an array instead of mutating - but I understand why that would hinder Signal adoption. However, considering the amount of time new Angular developers waste chasing down why their object mutation doesn't kick off change detection and all of the non-obvious object mutation bugs I've had to chase down over the years, maybe that trade-off would be worth it.

[JeanMeche]

Fwiw, ngrx reverted their change to freeze the state (ngrx/platform#4686)