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: deepWritableSignal #4582

Open
1 of 2 tasks
rainerhahnekamp opened this issue Nov 10, 2024 · 13 comments
Open
1 of 2 tasks

RFC: deepWritableSignal #4582

rainerhahnekamp opened this issue Nov 10, 2024 · 13 comments
Assignees

Comments

@rainerhahnekamp
Copy link
Contributor

rainerhahnekamp commented Nov 10, 2024

Update 10.12.2024: We’ve decided to pause this RFC for now, as the primary use case seems limited to forms. With the anticipated introduction of SignalForm in the future, we aim to avoid introducing a feature that might need deprecation in just 2–3 major releases.

However, if you can provide compelling examples of use cases beyond forms, we would be open to revisiting this proposal.

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

signals

Information

Problem

As Signal adoption grows, we anticipate that APIs will increasingly require Signals.
Type Signal will not cause any issues for the SignalStore, but WritableSignal will.

A good example we have already today, are Template-driven forms.

@Component({
    selector: "app-user",
    template: `<h1>User Detail</h1>
    <form (ngSubmit)="handleSubmit()">
      <input [(ngModel)]="firstname" />
      <input [(ngModel)]="city" />
    </form>`,
    standalone: true,
})
export class UserDetailComponent {
    #userStore = inject(UserStore);

    firstname = signal(this.#userStore.user.firstname());
    city = signal(this.#userStore.user.address.city());

    formUser = computed(() => ({firstname: this.firstname(), city: this.city()}))

    handleSubmit() {
        this.#userStore.save(this.formUser());
    }
}

We have to manually create a WritableSignal for each field, which doesn’t scale well.

Proposed Solution

deepWritableSignal makes all nested properties in an object literal within a WritableSignal into WritableSignals themselves. This approach matches the behavior of deepComputed, which can also be used as standalone.


deepWritableSignal can also be applied to an unprotected SignalStore and to the state returned by signalState. That would be the types WritableSignalSource and SignalState.

Example

Default Use Case

With the combination of linkedSignal, we provide a significant DX improvement without compromising the state protection:

@Component({
    selector: "app-user",
    template: `<h1>User Detail</h1>
    <form (ngSubmit)="handleSubmit()">
      <input [(ngModel)]="user.firstname" />
      <input [(ngModel)]="user.address.city" />
    </form>`,
    standalone: true,
})
export class UserDetailComponent {
    #userStore = inject(UserStore);

    user = deepWritableSignal(linkedSignal(this.#userStore));

    handleSubmit() {
        this.#userStore.save(this.user());
    }
}

Unprotected SignalStore

For an unprotected Signal Store

const UserStore = signalStore(
  {protectedState: false}, 
  withState({user: {
    id: 1,
    name: 'Konrad'
  }})
);

const userStore = new UserStore();

const user = deepWritableSignal(userStore.user);
user.name.set('Max');

For signalState

const userState = signalState({
  user: {
    id: 1,
    name: 'Konrad'
  }
});

const user = deepWritableSignal(userState.user);
user.name.set('Max');

Potential Extensions

Due to the protected state, every Signal from the SignalStore needs to be mapped to the type WritableSignal.

To address this, we could extend deepWritableSignal with an additional parameter that internally applies linkedSignal. Alternatively, we could implement an implicit linkedSignal if deepWritableSignal doesn’t receive a WritableSignal.

Version 1: Explicit linkedSignal

declare function deepWritableSignal<T>(signal: WritableSignal<T>, wrapWithLinkedSignal = false): DeepWritableSignal<T>;
declare function deepWritableSignal<T>(signal: Signal<T>, wrapWithLinkedSignal: true): DeepWritableSignal<T>;

export class UserDetailComponent {
    #userStore = inject(UserStore);

    user = deepWritableSignal(this.#userStore, true);

    handleSubmit() {
        this.#userStore.save(this.formUser());
    }
}

Version 2: Implicit linkedSignal

declare function deepWritableSignal<T>(signal: WritableSignal<T>, wrapWithLinkedSignal = false): DeepWritableSignal<T>;
declare function deepWritableSignal<T>(signal: Signal<T>): DeepWritableSignal<T>;

export class UserDetailComponent {
    #userStore = inject(UserStore);

    user = deepWritableSignal(this.#userStore);

    handleSubmit() {
        this.#userStore.save(this.formUser());
    }
}

Describe any alternatives/workarounds you're currently using

Alternatively, we could wait for Angular to introduce a DeepWritable. Until then, developers have to do more manual work.

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

  • Yes
  • No
@e-oz
Copy link
Contributor

e-oz commented Nov 12, 2024

Hello!
Great idea! Would be awesome to get in ngxtension too ;)
It will really help with forms. Thanks a lot!

1. Just a clarification, in case I miss or don't understand some parts.

In this example ("Default Use Case") I made some edits (marked with ⬅️), please let me know where I'm wrong and why:

@Component({
    selector: "app-user",
    template: `<h1>User Detail</h1>
    <form (ngSubmit)="handleSubmit()">
      <input [(ngModel)]="user().firstname" /> ⬅️
      <input [(ngModel)]="user().address().city" /> ⬅️
    </form>`,
    standalone: true,
})
export class UserDetailComponent {
    #userStore = inject(UserStore);

    user = deepWritableSignal(linkedSignal(this.#userStore.user)); ⬅️

    handleSubmit() {
        this.#userStore.save(this.user()); ⬅️
    }
}
Edits in the template:

I think that we should update the binding when user changes, not only user.firstname.
It's not a nitpicking or an attempt to "catch", I'm curious if there are hidden things that I miss or don't know that would make that user() non-needed.

Edits in the component:

Here I'm almost certain I missed some details, but maybe not :)

2. Linked signals in a protected store:

I think it should be implicit, because with an explicit approach, when that flag is set to false, things just will not work.

@markostanimirovic
Copy link
Member

A few questions came to my mind when I saw this proposal:

  • How to handle cases when set or update is the property name?

In the following example, form.user.set/update nested signals will conflict with set/update signal methods:

const form = deepWritable({
  user: { set: 0, update: 0 },
});
  • Do we need another API or we can upgrade signalState:
const form = signalState(
  { firstName: 'John', lastName: 'Lennon' },
  { writable: true },
);

form.lastName.set('Mayer');

@e-oz
Copy link
Contributor

e-oz commented Nov 12, 2024

@markostanimirovic, we can't read the value of a signal without calling a function, and getters here are not a solution, because then we will not be able to bind a signal to [(ngModel)].

So it will be

form().user().set.set(1)

or

form().user().set.update(v => !v)

Then, to read this value:

{{ form().user().set() }}

To bind:

[(ngModel)]="form().user().set"

@fancyDevelopment
Copy link

Hi guyes,
from my point of view an absolutely necessary thing we need! Since the beginning of signalStore I always wondered what is the easiest way of writing data back to signalStore. Having to create a method to update a single state property, even if I have no business logic connected with this update is too much effort, especially in a reactive programming style. Thus, already one year ago I came up with an own solution I have implemented in multiple projects already. The need came mainly from the work with template driven froms like @rainerhahnekamp mentioned.

Instead of just having a writeable or updateable signal I would like to extend the discussion to a "DeepPatchableSignal". Instead of just writing back single properties, we could also update full or parts of an object.

Using the example we have so far in this thread, according to my proposal we should have the following options to work with the state:

const user = toDeepPatchableSignal(store.user);
// patch parts of an object
user.patch({name: 'Max'});
// patch/update the full object
user.patch({id: 1, name: 'Max'});
// navigate down to deeper levels and be able to do the same
user.name.patch('John');

The goal is, that the patch function always take an Partial<> of the type of the object in the current hierarchy. Each property which is not part in the patch call remains unchanged. "Leaf signals" can of course also have the proposed set and update functions to bind those properties directly to ngModel.

From my point of view linkedSignal is not necessary to realize something like this. A clever concatenation of lambdas with the standard patchState function should also do the job if we find a way that toDeepPatchableSignal can somehow get access to the internal store object.

My current implementation can be found here:
https://github.com/angular-architects/ngrx-hateoas/blob/main/libs/ngrx-hateoas/src/lib/util/deep-patchable-signal.ts

Currently I am calling my toDeepPatchableSignal inside a method inside the store and returning the result with the help of a method. This looks currently like this and should be improved so that there is no need to create a method:

withMethods((store: any) => {
  const patchableUser = toDeepPatchableSignal<User>(newVal => patchState(store, { user: newVal }), store.user);
  return {
      getUserAsPatchable: (): DeepPatchableSignal<User> => {
          return patchableUser ;
      }
})

To be able to reuse this logic as easy as possible is put all this into a generic signal store feature you can find here:
https://github.com/angular-architects/ngrx-hateoas/blob/main/libs/ngrx-hateoas/src/lib/store-features/with-patchable-resource.ts

Would be really happy to see support of something similar build directly into signalStore. I would also offer to support during the implementation or the transfer of the ideas of my current solution.

@rainerhahnekamp
Copy link
Contributor Author

👋 @e-oz, thanks for your comments. Here are my answers.

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

If we call the value of the user signal, we just get the user. With just user.firstname we could activate that DeepWritableSignal.

handleSubmit() {
this.#userStore.save(this.user()); ⬅️
}

Thanks for pointing that out. I've updated the RFC, this was typo of mine

I think that we should update the binding when user changes, not only user.firstname.

Yes, when user's changes the firstname will be updated automatically.

I think it should be implicit,

I am also lending towards it.

@rainerhahnekamp
Copy link
Contributor Author

@markostanimirovic

How to handle cases when set or update is the property name?

If a nested object has a property like set or update, you would get a compilation error.

Do we need another API or we can upgrade signalState:

I don't see the need but I saw that my RFC used the signalStore instead the signalState. Fixed it now.

@rainerhahnekamp
Copy link
Contributor Author

@fancyDevelopment

like to extend the discussion to a "DeepPatchableSignal".

I like the idea, but I think adding a patch method doesn't follow the current style of the SignalStore, which works with standalone functions (tree shaking).

What do you think about integrating that into patchState?

const UserStore = signalStore(
  {protectedState: false}, 
  withState({user: {
    id: 1,
    name: 'Konrad'
  }})
);

and then within the store:

patchState(store => store.user, {id: 2})

That would be another PR though.

@gabrielguerrero
Copy link
Contributor

@rainerhahnekamp this is a great 🚀

@c-harding
Copy link

Hi @rainerhahnekamp, firstly thank you, this is a really cool idea. Secondly, here are a few thoughts of mine:

  1. Is this really deep? Can I go arbitrarily deep in the tree, or is the magic only applied one level deep?

  2. What about an alternative solution to the problem of set and update: keeping them in separate objects? The temporary solution I built myself looks more like this:

    readonly user = model<User>()
    protected readonly userLens = lens(this.user);

    userLens is a proxy object, and every property is a WritableSignal containing the value from user, in a two-way binding.

    .set() and .update() can still be called on user (and the appropriate signals will still be updated in userLens), but there’s no chance of a mix between the two. Also, I’m not sure if it was covered in your example, but using a proxy also allows signals to be created for elements that don’t yet exist in the base model, rather than all being initialised at start time. More specifically, a signal is only created when it is accessed.

    The catch with this solution is that two different properties are needed in the class, but the separation is clearer, and the lens will likely only be used in the template anyway. I implemented lens with Angular 17 effects performing two-way binding (though I’m looking forward to Angular 19 making this effects run sooner), I’m curious to see your implementation.

@e-oz
Copy link
Contributor

e-oz commented Nov 13, 2024

Perhaps some initial implementation would show us what is possible and what is not 👀

@rainerhahnekamp
Copy link
Contributor Author

Perhaps some initial implementation would show us what is possible and what is not 👀

@e-oz I agree. I haven't seen a complete objection so far. We can process with a prototype and gather feedback on that.

@fancyDevelopment
Copy link

@rainerhahnekamp

and then within the store:

patchState(store => store.user, {id: 2})

I think this is not of help to the ones who need this in template dirven forms approach. Here you want to mutate the state easily from the outside. Also in other cases i see this need again and again. So I believe the 'deep patchable signal' should be availalbe from outside the store. Otherwise we need to write wrapper methods again and again.

@rainerhahnekamp
Copy link
Contributor Author

@fancyDevelopment

I think this is not of help to the ones who need this in template driven forms approach.

No, it really isn't. It was meant to be a good feature for patchState in general.

That being said, we pause this RFC because we see the use case only for forms at the moment. Given that we will get a SignalForm in the future, we don't want to introduce a feature that has to be deprecated in 2 or 3 majors.

If you can come up with examples other than forms, we could start re-considering it.

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

6 participants