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

RFC: SignalStore with WritableSignals and ReadOnlySignalStore #4283

Open
1 of 2 tasks
marklagendijk opened this issue Mar 26, 2024 · 8 comments
Open
1 of 2 tasks

RFC: SignalStore with WritableSignals and ReadOnlySignalStore #4283

marklagendijk opened this issue Mar 26, 2024 · 8 comments

Comments

@marklagendijk
Copy link

marklagendijk commented Mar 26, 2024

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

signals

Information

When thinking about how Signals should change the Store concept, I thought: there should be a SignalStore.
So I googled for 'signal store' and found the beautiful implementation that has been created in NgRx.
There were two main features that I loved:

  1. It brings the power of 'composition over inheritance' that Signals bring us, to the Store world. And in a very powerful way, I might add.
  2. It cleverly builds on the existing Signal functionality, instead of doing too much itself.

However, I also found two points that were disappointing:

  1. A SingalStore service allows directly modifying the data from the outside. @rainerhahnekamp brought up this point in his excellent video about the SignalStore.
  2. Updating the data in the store is done via the patchState which is called with the Store instance, instead of being done via WritableSignal. This point goes against 'building on existing Signal functionality'.

This morning I woke up with an idea for tackling both these points. I do not know if:

  1. It is possible to implement this, or whether there is something I don't know about that is blocking this.
  2. It will be acceptable to introduce such a big change to the SignalStore. I would expect it would at least be a major version. Since the package is still in developer preview, it should be fine.

The idea:

  1. A SignalStore will hold DeepWritableSignals.
  2. All the withX calls get the SignalStore as argument, and can update any signal value via the normal set and update methods of those signals. For nested values this would look like this: store.person.address.street.set(value).
  3. The result of calling signalStore is not a SignalStore but a ReadOnlySignalStore. The ReadOnlySignalStore is a clone of its SignalStore, except that all it's signals are read only (asReadOnly)

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

  • Yes
  • No

I would love to, but I don't think it is feasible for me to do so.

@marklagendijk
Copy link
Author

marklagendijk commented Mar 26, 2024

This RFC proposes a different approach to #4210 feat(signals): Encapsulation

@rainerhahnekamp
Copy link
Contributor

@marklagendijk: I have a prototype for that DeepWritableSignals but it is far from stable. My use case was the support for the banana box syntax for Signals introduced in 17.2.

So if you have

const user = signalStore(withState({firstname: 'Hugo', lastname: 'Schmidt'}));

It would be great to have an easy option to use it in the template like

<input [(ngModel)]="user.firstname">
<input [(ngModel)]="user.lastname">

@marklagendijk
Copy link
Author

That could indeed be nice. Especially for component level stores.
I think having the option to expose the writable store (as you have in your PR) would be nice.

I think it could make sense to have these two options be separate functions. For example signalStore and writableSignalStore (probably needs a better name).

@Harpush
Copy link

Harpush commented May 3, 2024

@marklagendijk: I have a prototype for that DeepWritableSignals but it is far from stable. My use case was the support for the banana box syntax for Signals introduced in 17.2.

So if you have

const user = signalStore(withState({firstname: 'Hugo', lastname: 'Schmidt'}));

It would be great to have an easy option to use it in the template like

<input [(ngModel)]="user.firstname">
<input [(ngModel)]="user.lastname">

I think this is a great idea! Although it seems pretty complicated as you can't use computed here.

@rainerhahnekamp
Copy link
Contributor

@Harpush I think for the Form use case, it is not necessary to use computed. If yes, we are talking here about two different models. One "state model" and a "form model" which will eventually require a mapper in between.

@Harpush
Copy link

Harpush commented May 3, 2024

@Harpush I think for the Form use case, it is not necessary to use computed. If yes, we are talking here about two different models. One "state model" and a "form model" which will eventually require a mapper in between.

Maybe a graph flip is possible? Leaves will be signals while parents will be computed. Not entirely sure this is possible though

@rainerhahnekamp
Copy link
Contributor

@Harpush do you have an example? I'm afraid I don't fully get your point.

@Harpush
Copy link

Harpush commented May 3, 2024

@rainerhahnekamp For example if I have:

interface State {
  a: number;
}

const initialState: State ={
  a: 2
};

The logic will be (somehow dynamically if possible which I am not sure):

const a = signal(2)
const state = computed((): State => ({a: a()}));

So we have state() (readonly) and state.a() (writeable) at the end.
Not sure the best idea though as state becomes readonly.
Any other solution requires two state trees - for write and read I believe

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants