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

React to input changes: signal effects vs ngOnChange #55027

Open
mauriziocescon opened this issue Mar 25, 2024 · 7 comments
Open

React to input changes: signal effects vs ngOnChange #55027

mauriziocescon opened this issue Mar 25, 2024 · 7 comments
Labels
area: core Issues related to the framework runtime core: reactivity Work related to fine-grained reactivity in the core framework cross-cutting: signals
Milestone

Comments

@mauriziocescon
Copy link

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

core

Description

Often in angular, developers write code in order to react to input changes in components. In general, one of the common places used to achieve this goal is the life cycle hook ngOnChange.

Typically, something like:

ngOnChanges(changes: SimpleChanges): void {

   // a preliminary "if" lets you target only the input named value
    if (changes['value']) {
        // whenever value changes, do something
    }
}

Now, it seems to me the way to achieve the same goal with signals is using effects. So something like:

valueEffect = effect(() => {

    // the initial assignment makes sure the effect tracks value changes,
    //
    // even if test might not be needed in the untracked func (if you call 
    // a component method using directly this.value()).
    // 
    // Using untracked makes sure nothing else will be tracked except for value. 
    // Moreover, I can set other signals within untracked. 
    const test = this.value();

    untracked(() => {
       // whenever value changes, do something
    });
  });

Assuming the considerations above are correct (otherwise sorry for having missed some important points), I have the feeling the signal alternative is a bit strange. Say: effects have a wider scope compared to ngOnChange, but less intuitive to use for a very common thing like "react to an input change".

I'd personally like to have a more clean approach where (possibly) I clearly state that "an effect should only track this specific signal(s) changes and react only to that".

Recently, I've noticed there've been some tickets around the same topic: #54985 and #54372.

Here is a not trivial example to illustrate the differences: https://stackblitz.com/edit/stackblitz-starters-ey1c9p

Proposed solution

I'd personally like to have a more clean approach where (possibly) I clearly state that "an effect should only track one(many) specific signal(s) changes and react only to that".

Alternatives considered

It's mostly a matter of making the intention "react to a specific input change" more clear... no other thing. And again: if I missed some important points, sorry!

@sonukapoor sonukapoor added area: core Issues related to the framework runtime cross-cutting: signals labels Mar 25, 2024
@ngbot ngbot bot added this to the needsTriage milestone Mar 25, 2024
@JeanMeche
Copy link
Member

Hi, about explicit dependencies we had a previous discussion in #50916

@mauriziocescon
Copy link
Author

mauriziocescon commented Mar 25, 2024

Indeed... also noticed #54548 and the comment from the ngrx guys!
#54548 (comment)

Well, don't know what else to say then :-)... just that the automatic-tracking design choice for effects (with the addition of untracked) sounds a bit strange to me. It sounds kind of in between "full responsibility on developers" vs "everything automatic".

Moreover, IMHO explicit deps would make the code very clear on any developer's intention.

But ok: it might be simply my attitude (always: freedom / responsibilities to devs with great doc :-)) and the fact I'm missing an extensive usage of signals in a real (and big) app.

@Harpush
Copy link

Harpush commented Mar 26, 2024

@mauriziocescon you can also take my implementation from here: #54614 (comment)

@pkozlowski-opensource
Copy link
Member

So there are multiple topics being discussed here, ranging from working with inputs all the way to the automatic dependencies tracking with signals.

Let me start with re-iterating our position from #50916 - we've chosen signals partly because of their automatic tracking nature: we don't want developers to remember and deal with the friction of manual dependency tracking. Having said this people are free to use coding patterns that make dependencies more explicit if this help their code organization.

Now, for the input as signals I would say that reactive primitives (computed, effect) are the way to go. Most of the time we expect people to reach out for a computed and only use effect for truly side-effectful operations. In this sense your example could be re-written as follows:

valueEffect = effect(() => {
       // whenever value changes, do something
       doSomething(this.value());
  });

I don't think untracked is needed by default since:

I'm happy to discuss more around more concrete examples - here we do have just a short code snippet and this makes it impossible to reason about the general logic

@pkozlowski-opensource pkozlowski-opensource added the core: reactivity Work related to fine-grained reactivity in the core framework label Mar 27, 2024
@mauriziocescon
Copy link
Author

mauriziocescon commented Mar 27, 2024

Hey!

Thanks for the reply: much appreciated. Well, I provided a stackblitz at the beginning, but it's just an easy example that can be refactored the way you proposed.

Considering the inputs part clarified, let me try to make the automatic deps tracking more clear: the logic and functionalities currently available for effects covers all the needs. No disagreement on that. So there isn't anything logically unsupported.

The problem I see is not about the logic, but what developers will use the most on a daily basis and how effects behave by default. IMHO effects will be used a lot (not just rarely), but they might create problems (out of the box) for common developer needs.

// `doSomething` could do many complex things like calling a method on a global service that
//
// 1) changes a signal as its internal implementation
//  (rxjs optional, signals replacing subjects as "state"), 
// 
// 2) that reads some service internal signals to perform some operations 
// (unwanted deps added to valueEffect by mistake)
// 
// 3) that is not under my control in terms of 
// implementation / refactoring (huge enterprise app with ~100 devs)



// default (automatic deps tracking, safe guard on signal writes): I'd say fine 
// if doSomething is simple and under my control. It tracks any dep (wanted or unwanted)
// coming from doSomething. 
valueEffect = effect(() => doSomething(this.value()));


// variation (safe guard on the tracked dep(s), can write on signals): great when I know 
// I don't need any dep from doSomething and I don't care about its implementation. 
valueEffect = effect(() => {
       this.value(); // <- simple reactive condition I want to be auto tracked
       untracked(() => doSomething(this.value()); // <- safe callback
  });

I personally believe that the watch func suggestion (see comment) is what is missing right now. No need to change the effect func (perfect in some cases), just the addition of a straightforward way of expressing the "effect variation" above and good documentation (vue.js: watch vs watchEffect).

Here is a stackblitz with the service example mentioned above and lots of comments https://stackblitz.com/edit/stackblitz-starters-csmdrr

Again: the code could be refactored and problems solved. But don't forget about the real life:

  • lots of developers,
  • big app,
  • complexity,
  • calling global services without knowing the internal implementation,
  • junior devs,
  • the quick fix suggestion coming from Error: NG0600 (by the way: why suggesting to enable allowSignalWrites instead of a page untracked vs allowSignalWrites? :-) ) .

In other words: in a world of signals (services / components / ... ) is it really rare writing to signals in effects while reacting (for example) to component input changes? Is the default effect usage the safest in terms of unwanted tracked deps?

Just my 2 cents!

PS: thanks a lot for the amazing work you're doing!

@imaksp
Copy link

imaksp commented Apr 3, 2024

Hi all, want to add one more common use case, what if I need to call an api function based on some signal field from common store, & that api function will write to unrelated signals, here what could be best option untracked or effect with allowSignalWrites ? (I assume untracked will be best option).

so I think even if untracked usage is discouraged I think many people will need to use it, correct me if I am wrong.

@mauriziocescon
Copy link
Author

mauriziocescon commented Apr 3, 2024

@imaksp as of today:

// tracks value and any signal_read inside doSomething; 
// in case of signal write inside doSomething you get NG0600
valueEffect1 = effect(() => doSomething(this.value()));

// tracks value and any signal_read inside doSomething; 
// no problem writing on signals inside doSomething
valueEffect2 = effect(() => doSomething(this.value()), { allowSignalWrites: true });

// tracks only value; 
// no problem writing on signals inside doSomething
valueEffect3 = effect(() => {
        this.value();
        untracked(() => doSomething(this.value()));
});

Some recent news somehow related to the topic: https://github.com/proposal-signals/proposal-signals

Still think a watch (framework) func might be handy and avoid confusion... maybe with some rework on the NG0600 error message suggestions!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: core Issues related to the framework runtime core: reactivity Work related to fine-grained reactivity in the core framework cross-cutting: signals
Projects
None yet
Development

No branches or pull requests

6 participants