@preact/signals useComputed behavior can be unexpected

[jviide]

• Check if updating to the latest version resolves the issue

Environment

• I am using @preact/signals-core
• I am using @preact/signals
• I am using @preact/signals-react
  • I understand usage changed in v2, and I've followed the React Integration instructions

Describe the bug

The current @preact/signal's useComputed implementation may lead to unexpected behavior when the signals in the compute function's captured scope change. This can make reasoning about useComputed updates hard.

To Reproduce

The following link demonstrates the issue: https://stackblitz.com/edit/vitejs-vite-2jxwcfy5?file=src%2Fapp.tsx

1. Observe that the shown value is "1".
2. Click on the "update s1" button, observe that the value is now "2".
3. Click on the "depend on s2" button, observe that the value is still "2".
4. Click on the "update s2 button", observe that the value is still "2".
5. Click on the "update s1 button", observe that the value is now "aa".

As a test-ish pseudocode the flow goes something like this:

function Component({ sig }) {
    return useComputed(() => sig.value);
}

const s1 = signal(1);
const s2 = signal("a");

render(<Component sig={s1} />, scratch);
// now scratch.textContent is "1", the computed depends on s1.

s1.value = 2;
rerender();
// now scratch.textContent is "2", the computed depends on s1.

render(<Component sig={s2} />, scratch);
// scratch.textContent is still "2", the computed still depends on s1.

s2.value = "aa";
rerender();
// scratch.textContent is still "2", the computed still depends on s1.

s1.value = 3;
rerender();
// scratch.textContent is now "aa", the computed depends on s2.

Expected behavior

There are two main ways to make this behavior more consistent.

1. Make the compute function one of the useComputed dependencies, and update the dependency value on each recompute. This allow the computed to capture potential new signal dependencies (and other non-signal values from the surrounding scope, e.g. prop changes). If the compute function changes on rerender and something is depending on the computed returned by useComputed, then this causes a recompute down the line.

   function Component({ sig }) {
       return useComputed(() => sig.value);
   }
   
   const s1 = signal(1);
   const s2 = signal("a");
   
   render(<Component sig={s1} />, scratch);
   // now scratch.textContent is "1", the computed depends on s1.
   
   render(<Component sig={s2} />, scratch);
   // now scratch.textContent is "a", the computed depends on s2.
   
   s1.value = 2;
   rerender();
   // scratch.textContent is still "a", the computed still depends on s2.

   The main downside is that the compute function may run more often than currently. However, it can be argued that one of the points of signal is to avoid these components that are "roots" for signals/computeds from rerendering in the first place, which tends to reduce the effect. For the cases where where performance is tantamount, this can be avoided with the familiar useCallback pattern:

   function Component({ sig }) {
     const fn = useCallback(() => sig.value, [sig]);
     return useComputed(fn);
   }

   Another downside is that the behavior for cases where the options argument changes is still a bit unclear.

   As a purely personal opinion, regardless of those downsides, I'd argue that this is the approach of least surprise. YMMV of course.

2. Always use the initial compute function. This way useComputed essentially becomes useState(() => computed(compute, options))[0].

   function Component({ sig }) {
       return useComputed(() => sig.value);
   }
   
   const s1 = signal(1);
   const s2 = signal("a");
   
   render(<Component sig={s1} />, scratch);
   // now scratch.textContent is "1", the computed depends on s1.
   
   render(<Component sig={s2} />, scratch);
   // scratch.textContent is still "1", the computed depends on s1.
   
   s1.value = 2;
   rerender();
   // now scratch.textContent is "2", the computed still depends on s1.

   The current version of useLiveSignal utility can be used to make useComputed react to dependency changes:

   function Component({ sig }) {
     const $sig = useLiveSignal(sig);
     return useComputed(() => $sig.value.value);
   }

A third option would be to explain the current behavior in documentation. That said, I personally think the current behavior doesn't have upsides compared to either of the alternatives described above.

I submitted two pull requests, each implementing one of the alternative approaches described above. #754 implements alternative 1 ("recompute when the compute function changes"). #755 implements alternative 2 ("always use the initial compute function"). Both PRs add tests for the relevant behavioral changes.