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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(signal_effect with explicit deps): introduce a new "watch" func having explicit signal_deps and skipFirstCallback #55592

Closed
mauriziocescon opened this issue Apr 30, 2024 · 4 comments

Comments

@mauriziocescon
Copy link

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

core

Description

Statement

Adding the possibility of defining explicit deps in signal effects is (already) an old story 馃槄. There are many tickets directly or indirectly touching the point (for example I opened one in the past #55027 related to ngOnChange).

The thing is: there's nothing currently missing in terms of functionalities, but I believe the current way of implementing some patterns requires effects in a particular shape. And IMHO this opens up the opportunity to provide a standard solution at framework level and update the "scary" doc on the effect usage 馃槄.

Pattern example

Here is a very common case: I'd like to create a service powered by signals where any time some parameters change, there's a new fetch of items. The implementation should take advantage of the reactive pattern.

This is a common request easily implementable with observables. Here is a possible version with signals: https://stackblitz.com/edit/stackblitz-starters-lt4skr

Problem

Let's focus on the important part of the code from the stackbliz:

    
  // service prop
  //
  // 1) I need a reactive condition: wants to track only param changes
  //
  // 2) when params changes, I want to fetch data and
  // signal.write items. At the same time I don't want
  // to have other signals.read causing potential problems
  // (not interested in anything else than params).
  //
  // 3) effects are always executed once: but I actually
  // don't want to fetch data based on the initial value
  // of params... only when updateParams is called.
  // So, I need a check in order to skip
  // the first execution.
  //
  private paramsDidChange = effect(() => {
    this.params();

    untracked(() => {
      if (!this.skipCallback) {
        this.loading.set(true);
        this.fetchItems(this.params())
          .then((items) => this.items.set(items))
          .catch((err) => this.error.set(err))
          .finally(() => this.loading.set(false));
      }
      this.skipCallback = false;
    });
  });

  // consumers of the service call this public method
  // triggers the reactive chain
  //
  updateParams(params: { textSearch: string }): void {
    this.params.set({ textSearch: params.textSearch });
  }

Or in few words (there might be slightly different alternatives):

  private paramsDidChange = effect(() => {
    
    // reactive condition to track
    this.params();

    // can signal.write and signal.read without issues
    untracked(() => {
      // skip the first effect iteration
      if (!this.skipCallback) {

        // signal.write has the same risks of creating loops like with obs
      }
      this.skipCallback = false;
    });
  });

IMHO this code is kind of unavoidable and not straightforward to address developer needs.

Proposal: introduce a dedicated watch func

With a dedicated watch func, the code above could be better written like this:

// params is the only condition to track
private paramsDidChange = watch(this.params(), () => {
  // can signal.write and signal.read without issues

  // lazy execution
  // by default, the callback is not immediately executed
  // a dedicated flag can toggle the behaviour 
});

Now, you might argue that the effect-pattern above is not really used. But I kind of disagree with this statement: it's just hidden down the chain of code. Some examples:

This is also a common pattern proposed by vue.js: watchers. In general, I frankly don't know how I could solve the very common problem above without using it.

IMHO the usage of effects cannot be rare (as stated by the doc) because of the missing watch func. And the current usage is not ideal.

Finally: thanks a lot for the amazing work you're doing! Angular rocks more than ever before and it's a pleasure to work with it. Bravi!

Proposed solution

Introduce a watch (or equivalent) func targeting the common pattern

  private paramsDidChange = effect(() => {
    
    // reactive condition to track
    this.params();

    // can signal.write and signal.read without issues
    untracked(() => {
      // skip the first effect iteration
      if (!this.skipCallback) {

        // signal.write has the same risks of creating loops like with obs
      }
      this.skipCallback = false;
    });
  });

Alternatives considered

Keep using effect

@JeanMeche
Copy link
Member

Hi, the subject of explicit effect dependencies was already discussed here: #50916.

@mauriziocescon
Copy link
Author

mauriziocescon commented Apr 30, 2024

I know, it's discussed in many places directly or indirectly.

But in #50916, the idea is to modify the current effect function... which I personally don't like as an approach. Let's keep effect as it is.

What I'd ideally like to discuss is the following:

  • is it really true that effects can be rarely used?
  • what are the official counterarguments for not introducing a watch func?

As I tried to report above, my view is that the usage of effects_untrack is just kind of hidden and highly necessary (and used a lot) for common needs.

It's just that we're still mixing signals / observables... so it's not really clear. But with only signals, I have the feeling the story would be different.

IMO effects would turn (almost) unnecessary with the addition of a watch func.

@pkozlowski-opensource
Copy link
Member

@mauriziocescon this issue essentially re-iterates what was already described in #55027. We hear you but I don't think an additional issue gets us any closer to the solution - what you are describing is well understood.

Let's centralize the discussion in #55027 which represents a real-life use-case.

@pkozlowski-opensource pkozlowski-opensource closed this as not planned Won't fix, can't repro, duplicate, stale Apr 30, 2024
@mauriziocescon
Copy link
Author

Ok, my bad... just started integrating signals and I find myself using the pattern above in many places! 馃槀

Gonna create a wrapper for now... 馃槀

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

No branches or pull requests

3 participants