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

Timing issue with useComputed #315

Open
jakearchibald opened this issue Mar 4, 2023 · 3 comments
Open

Timing issue with useComputed #315

jakearchibald opened this issue Mar 4, 2023 · 3 comments
Labels
bug Something isn't working

Comments

@jakearchibald
Copy link

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

JoviDeCroock commented Mar 8, 2023

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

jakearchibald commented Mar 9, 2023

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 😄

@JoviDeCroock JoviDeCroock added the bug Something isn't working label Apr 3, 2024
@jviide
Copy link
Contributor

jviide commented May 24, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants