angular / angular

Deliver web apps with confidence 🚀
https://angular.dev
MIT License
94.71k stars 24.68k forks source link

Computed signals and Effects should produce runtime warning if dependencies reaches zero #55647

Open jonahbron opened 2 weeks ago

jonahbron commented 2 weeks ago

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

Core (signals)

Description

An easy pitfall to encounter with computed signals and effects is creating a function that only gets called once because it depends on non-signal conditions. Especially for devs new to signals (almost everyone at this point), it can be a counter-intuitive problem source to debug. And there are already issues reported that can be traced to this.

https://github.com/angular/angular/issues/51180

Here's some example code that I wrote that caused non-obvious problems for me:

constructor() {
  effect(() => {
    this.calendarRef?.setInput('selected', this.range());
  });
}

this.range is a signal, and this.calendarRef is possibly undefined. I expected any changes to range to trigger the effect, but the optional chaining on the calendarRef (which was undefined) prevented range() from ever being evaluated and thus tracked as a dependency. And I believe that even if it wasn't undefined for the first run, if it ever reached undefined while the effect was evaluated, the effect would never run again.

Proposed solution

At the least, if the first run of a computed() or effect() callback produces no dependencies (invokes no signals), a runtime warning should show in the console to warn the user that their computed signal/effect will not run again.

It might also be useful to warn them if their dependency count drops from >1 to 0, which again means the computed signal/effect will not be evaluated again.

Similar to allowSignalWrites, the dev could remove these training-wheels warnings by positively expressing that they are intentionally designing a callback that will only be called once/stop being updated at some point.

Alternatives considered

It would likely be harder to implement, but static code analysis could detect the possibility of a zero-dependency computed signal/effect.

Regardless of whether a warning is added, documentation talking about this pitfall would be useful; currently not present as far as I can tell.

jonahbron commented 2 weeks ago

For reference to anyone else who runs into this issue in the meantime, there are a couple of solutions; one is to ensure range is always checked.

  effect(() => {
    const range = this.range();
    this.calendarRef?.setInput('selected', range);
  });

But I felt that this solution could be brittle; someone (even myself in the future) could easily come along and think that factoring out that variable is okay. That could be defended against with a comment like this that standardizes the practice of evaluating dependent signals unconditionally:

  effect(() => {
    const range = this.range();
    // ^ Evaluate all signals first ^
    this.calendarRef?.setInput('selected', range);
  });

But the solution I settled on instead was to turn calendarRef itself into a signal. Then the optional chaining all works as expected.

  effect(() => {
    this.calendarRef()?.setInput('selected', this.range());
  });