preactjs / signals

Manage state with style in every framework
https://preactjs.com/blog/introducing-signals/
MIT License
3.64k stars 89 forks source link

Timing issue with useComputed #315

Open jakearchibald opened 1 year ago

jakearchibald commented 1 year ago

https://codesandbox.io/embed/charming-rosalind-qyxbh1?file=/src/index.js&codemirror=1

Here, a component is only rendered if the value of a signal is not-null.

Within that component, a useComputed assumes the signal is not-null. However, the recomputation appears to run before the component is unmounted, meaning it receives a null value.

JoviDeCroock commented 1 year ago

Hmm yes, this seems to be a side-effect of mixing a synchronous update with an asynchronous one. When we can't do an optimal update (like synchronously updating text or an attribute) we have to fall back to re-rendering the VNode, which in this case leads to changing the return value from the VNode to that dom-node.

However the dependencies does include one that we can do synchronously i.e. that recompute which will happen synchronously with the null value while the VNode update will be scheduled like any other update in Preact, which would allow us to batch it with useState/...

Trying to reason at the moment about a more intuitive way to go about this.

EDIT: I guess it could be more intuitive if we say that computeds used as hooks are hooked into the Preact render cycle

jakearchibald commented 1 year ago

I guess it could be more intuitive if we say that computeds used as hooks are hooked into the Preact render cycle

I think that's probably the safest thing. I ran into another bug that might also be related to timing. I'll file it today. Edit: nope the other bug was my fault 😄

jviide commented 1 month ago

How about something like this? Sorry, wrote this in JavaScript instead of TS so I could test it quickly in the sandbox:

function useComputed(compute) {
  // Keep track of the latest compute function.
  const $func = useRef(compute);
  $func.current = compute;

  // Keep track of the latest compute function result
  // so that we can fall back to it after unmount.
  const $value = useRef();

  // On unmount just forget the compute function and 
  // start returning the last computed value.
  useEffect(() => {
    () => {
      $func.current = null;
    };
  }, []);

  const $compute = useRef();
  if (!$compute.current) {
    $compute.current = computed(() => {
      if ($func.current) {
        $value.current = $func.current();
      }
      return $value.current;
    });
  }

  // Update the internal flag here?
  // (currentComponent as AugmentedComponent)._updateFlags |= HAS_COMPUTEDS;
  return $compute.current;
}

This way the computed's updates end in tandem with the component's lifecycle, and it'll just start returning the last computed value whenever asked.

Edit: Yeah that won't solve the problem completely either.