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

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

Open
divdavem opened this issue Jun 7, 2024 · 7 comments

Comments

@divdavem
Copy link

divdavem commented Jun 7, 2024

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):

  • having a way to intercept tracked reads (as suggested by @shaylew for a different issue here)
  • having a way to know whether a computed signal is dirty (i.e. one of its direct or transitive dependencies have changed since the last computation)
  • having a way to know whether a computed signal actually changed after recomputing it (taking into account its own equals function)

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
Copy link

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

@divdavem
Copy link
Author

divdavem commented Jun 7, 2024

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
Copy link
Collaborator

@divdavem have you tried using untrack within the computed?

@divdavem
Copy link
Author

divdavem commented Jun 7, 2024

@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
Copy link
Collaborator

how would that be implemented algorithmically?

@divdavem
Copy link
Author

how would that be implemented algorithmically?

@NullVoxPopuli I have opened proposal-signals/signal-polyfill#19 to show how this could be implemented.

@robbiespeed
Copy link

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();
});

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

4 participants