RFC: Limit direct state updates outside a SignalStore

[markostanimirovic]

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

signals

Information

Currently, SignalStore's state can be directly updated from outside:

const CounterStore = signalStore(withState({ count: 0 }));

class CounterComponent {
  readonly store = inject(CounterStore);

  setCount(count: number): void {
    patchState(this.store, { count });
  }
}

While this approach provides flexibility (and reduces boilerplate 😛), it leaves room for abuse. On the other hand, state encapsulation provides better control which is important for complex systems.

There are two options to limit direct state updates outside a SignalStore:

1) Introducing the ~readonlyState~ protectedState flag and having compilation error when the state is updated outside a SignalStore:

const CounterStore = signalStore(
  { protectedState: true },
  withState({ count: 0 }),
  withMethods((store) => ({
    set(count: number): void {
      patchState(store, { count }); // ✅
    }
  }))
);

class CounterComponent {
  readonly store = inject(CounterStore);

  setCount(count: number): void {
    patchState(this.store, { count }); // ❌ compilation error
    this.store.set(count); // ✅
  }
}

The open question is - should we have protectedState: true by default?

2) Introducing the ESLint rule for state updates outside a SignalStore. However, with ESLint, it won't be possible to cover all scenarios. For example, when patchState is used in a separate function:

function setCount(countState: StateSignal<{ count: number }>, count: number): void {
  patchState(countState, { count });
}

const CounterStore = signalStore(
  withState({ count: 0 }),
  withMethods((store) => ({
    set(count: number): void {
      setCount(store, count); // ✅
    }
  }))
);

class CounterComponent {
  readonly store = inject(CounterStore);

  setCount(count: number): void {
    setCount(this.store, count) // ❌ ESLint cannot catch this
  }
}

Describe any alternatives/workarounds you're currently using

No response

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

• [X] Yes
• [ ] No

[LMFinney]

I like option 1 with "true" as the default, because it has always felt weird that this was even possible.

However, a default of "false" would probably be a lot easier for backwards compatibility.

[michael-small]

The open question is - should we have readonlyState: true by default?

Yes. I think @LMFinney's point above encapsulates that well.

That said, I think refactors which would be needed to fix instances like the patchState(this.store, { count }) --> this.store.set(count) would be fairly straight forward if not a bit of busy work for application developers. I would just ensure there is a clear error and link to an example fix like this one. Disclaimer: I don't use Signal Store more than a few little side projects, but am a component store user in production apps who follows issues like this. So it's easy for me to say "just break backwards compatibility lol"

[markostanimirovic]

Backward compatibility is not a big problem in my opinion. If we decide to go with readonlyState: true by default, the migration script that adds readonlyState: false to all existing stores will be provided for a smooth upgrade.

The question is more about: are direct state updates frequent/usual in your codebases?

[anthony-b-dev]

It seems to me that if we draw some inspiration from classic @ngrx/store, that the readonlyState: true by default is preferable, requiring developers to explicitly create a method within the store as a way of constraining what could become a free for all with multiple people working on larger projects and interacting with the store.

[michael-small]

Backward compatibility is not a big problem in my opinion. If we decide to go with readonlyState: true by default, the migration script that adds readonlyState: false to all existing stores will be provided for a smooth upgrade.

Good stuff. Reminds me of how typed forms were rolled out in a way. Low stakes, still compatible and ultimately something where you could rip out the changes from the migration to make behavior more consistent.

The question is more about: are direct state updates frequent/usual in your codebases?

In my component store use cases we don't tend to do direct state updates. That's mostly from following what we think are best practices in NGRX in general. Would we shift to using signal store, I think we would continue this pattern directly with patching in the store only. I would hope by that time we may migrate that this is the default implementation and best practice.

[BaronVonPerko]

I also prefer option 1. It makes sense that the store does not allow updates outside of itself, and makes the dev explicitly state that they want to overwrite those rules.

[e-oz]

That's a really good move! My vote: Option 1 with true by default.

[jaromir-roth]

I also prefer option 1 with true by default. However, I am not sure the property name readonlyState describes the exact purpose. Maybe something like allowExternalStateUpdates could be more self-descriptive?

[gabrielguerrero]

I like it, readonlyState true by default I think is the best.

[rainerhahnekamp]

Yes, readonlyState true by default. Would be great if we get that in the first release.

[manfredsteyer]

I'd also prefer readonlyState true by default as IMHO one of the most important jobs of a store is ensuring that state is only updated in a well-defined way.

[santoshyadavdev]

Option 1 with readonlyState true sounds better option.

[mauriziocescon]

As all the others: 1) + readonlyState: true by default!

I'd even go further and foreseen a near future where readonlyState is not even present (it's true for everybody without choosing). Meaning you can only call patchState inside SignalStore.

signalState as a field in a service can cover readonlyState: false cases... as it can now cover the lack of encapsulation problem!

PS: thanks a lot for the RFC! Great job!

[jits]

As is the consensus, my vote would be for option 1 — readonlyState: true. And as suggested by @mauriziocescon, that would be the default if no config option is given in the signal store definition.

---

One scenario I'd like put out there, for consistency: allowing patchState to still be called in a class definition that extends the signal store definition class. Example:

const _MyStore = signalStore(
  // …
);

@Injectable()
export class MyStore extends _MyStore {
  // I can still call `patchState(…)` in methods, etc. within this class definition
}

[rainerhahnekamp]

💯 for default/automatic, if not defined otherwise. that's excellent @mauriziocescon.

migration schematics could add the explicit readonlyState: false

[GuillaumeNury]

Why not a writableState: false by default?

1. We already have WritableSignal in Angular APIs

2. No need to initialize property (undefined is a falsy value)

Anyway, option 1 is better 🙂

[brandonroberts]

I refuse to support any effort to reduce boilerplate ... But I digress 😄

It looks good to me, but I think protectedState: true would be a better name and aligns more consistently with a protected class property that you can't update from outside.

[markostanimirovic]

Thanks Brandon! protectedState sounds better to me too 💯

[mauriziocescon]

Recap:

• const CounterStore = signalStore(...) => encapsulated,
• const CounterStore = signalStore({ protectedState: true }, ...) => encapsulated,
• const CounterStore = signalStore({ protectedState: false }, ...) => NOT encapsulated.

Right?

[markostanimirovic]

Recap:

• const CounterStore = signalStore(...) => encapsulated,
• const CounterStore = signalStore({ protectedState: true }, ...) => encapsulated,
• const CounterStore = signalStore({ protectedState: false }, ...) => NOT encapsulated.

Right?

Correct 👍

[mauriziocescon]

I wonder if it's worth supporting protectedState: false on the long run...

I mean: except for migrating existing code (so nothing gets broken), what would be the use cases for protectedState: false? Assuming you want to document the feature...

Of course the usual one: freedom to choose! But still...

[gabrielguerrero]

I think the use cases for protectedState: false will be mostly examples or small code to showcase something like in stackblitz

[rainerhahnekamp]

About protectedState:

protected means the value is only available to dervied classes. readonly means the value is available to everyone but I can't change it.

Now you could argue that you are hiding the inner Signal and that's why it is protected, e.g. really hidden. To me, that's an implementation detail.

For end-users though, the state is there but I can change it. Hence readonly...

[gabrielguerrero]

I think what they mean by protected is that it is read-only outside the store but can be changed inside the store, whereas readyOnly might confuse the user into thinking they can never change it

[rainerhahnekamp]

protected is that it is read-only outside the store but can be changed inside the store

Oh yeah, that makes sense. We have to see it from the perspective of the store's author and not of the consumer. They just see the service.