tc39 / proposal-signals

A proposal to add signals to JavaScript.
MIT License
3.42k stars 59 forks source link

How to allow `set` in a `Computed` to trigger a recomputation synchronously (until the result is stable) #226

Open divdavem opened 5 months ago

divdavem commented 5 months ago

Hello, As a maintainer of the tansu signal library, I am trying (here) to re-implement it using signal-polyfill and I came across the following difference of behavior, that I would like to solve, avoiding any breaking change.

With tansu, calling set on a signal in a computed that has a dependency on that signal re-triggers the computation of the computed until the value is stable (with a fixed maximum number of iterations). For example:

import {writable, computed} from "@amadeus-it-group/tansu";
const s = writable(0);
const c = computed(() => {
  const value = s();
  if (value < 10) {
    s.set(value + 1);
  }
  return value;
});
const d = computed(() => {
  const value = s();
  if (value < 10) {
    s.set(value + 1);
  }
  return value;
});
console.log(c()); // logs: 10
console.log(c()); // logs: 10
console.log(d()); // logs: 10
console.log(d()); // logs: 10
console.log(c()); // logs: 10
console.log(d()); // logs: 10

(cf this test)

With signal-polyfill, there was apparently a different design decision:

import { Signal } from 'signal-polyfill';
const s = new Signal.State(0);
const c = new Signal.Computed(() => {
  const value = s.get();
  if (value < 10) {
    s.set(value + 1);
  }
  return value;
});
const d = new Signal.Computed(() => {
  const value = s.get();
  if (value < 10) {
    s.set(value + 1);
  }
  return value;
});
console.log(c.get()); // logs: 0
console.log(c.get()); // logs: 0
console.log(d.get()); // logs: 1
console.log(d.get()); // logs: 1
console.log(c.get()); // logs: 2
console.log(d.get()); // logs: 3

My question is simple: how may I reproduce the behavior of tansu when implementing it using signal-polyfill?

Do you think this signals proposal could change to adopt this different behavior?

Alternatively, we could probably implement this if we had some other primitives that are missing in the current specification (and I think those would be useful anyway):

This way, maybe we could have a loop in our tansu computed which, after each computation, goes over all tracked reads called during the computed and checks if any of them is dirty at the end of computed. If it is the case, it means one of the (direct or transitive) dependencies changed during the call to computed and that we may need to recompute (in case those dependencies really changed).

What do you think?

szagi3891 commented 5 months ago

I believe that mutation during computation should be completely prohibited. What is in the computed should be a pure function.

divdavem commented 5 months ago

I believe that mutation during computation should be completely prohibited. What is in the computed should be a pure function.

@szagi3891 Thank you for expressing your opinion. It is indeed a best practice to only have a pure function as the callback for computed.

However, I am against completely prohibiting calling set in computed. The Signal.Computed function described in this spec can be used to implement effects too (and effects are usually not pure functions). But the goal of this issue is not to debate whether calling set should be completely prohibited inside computed. There is this issue for that purpose

NullVoxPopuli commented 5 months ago

@divdavem have you tried using untrack within the computed?

divdavem commented 5 months ago

@NullVoxPopuli Thank you for your answer!

@divdavem have you tried using untrack within the computed?

Do you mean around s.get()? But I would still like the computed to be recomputed again when s changes.

NullVoxPopuli commented 5 months ago

how would that be implemented algorithmically?

divdavem commented 5 months ago

how would that be implemented algorithmically?

@NullVoxPopuli I have opened https://github.com/proposal-signals/signal-polyfill/pull/19 to show how this could be implemented.

robbiespeed commented 5 months ago

The current behaviour of only invalidating the computed makes sense to me, allowing set inside a computed seems fine, but I don't see benefit in automatically recomputing if a dep changes inside the computed.

If you want some kind of mutation of deps to affect the result like your example you can use untrack. Is there a use case you can provide that this solution wouldn't work for?

const c = new Signal.Computed(() => {
  Signal.subtle.untrack(() => {
    if (s.get() < 10) {
      s.set(10);
    }
  });
  return s.get();
});