pmndrs / valtio

🧙 Valtio makes proxy-state simple for React and Vanilla
https://valtio.dev
MIT License
9.05k stars 251 forks source link

This library doesn't seem to be glitch free? #296

Closed ziriax closed 2 years ago

ziriax commented 2 years ago

First of all, thanks for sharing this very nice library!

I was doing some experiments, and I noticed that unlike most "push/pull functional reactive programming" libraries, this library is not glitch free, e.g. the callbacks of derived computations can be called with out-of-sync values, producing intermediate invalid results, and can be invoked multiple times per "leaf node" change, which can be inefficient in large "dataflow graphs".

Is this by design? To workaround this, one can apply ancient synchronous dataflow techniques, like giving each node in the dataflow a rank and using a priority queue to invoke the callbacks in the correct order. A bit like a parallel CPU stack.

Code example at https://stackblitz.com/edit/react-sqyzte

import React from "react";
import "./style.css";

import { proxy, useSnapshot } from 'valtio'

import { derive } from 'valtio/utils'

const state = proxy({ value: 0 });

// derived1 = state
const derived1 = derive({
  value: (get) => get(state).value,
})

// derived1 = derived2
const derived2 = derive({
  value: (get) => get(derived1).value,
})

// derived3 = state + derived1 - derived2 = state + derived1 - derived1 = state + 0 = state
const derived3 = derive({
  value: (get) => {
    // notice in the console that this callback is invoked TWICE per state update.
    const v0 = get(state).value;
    const v1 = get(derived1).value;
    const v2 = get(derived2).value; // notice in the console that v2 is out-of-sync in the first callback invocation
    const r = v0 + (v1 - v2);
    // notice the glitches in the console
    // when state is changed, this callback is invoked TWICE with out-of-sync values.
    // glitches make debugging really hard, and can cause invalid intermediate values like NaN, div by zero, etc...
    console.log(`v0=${v0} v1=${v1} v2=${v2} => r=${r}`);
    return r;
  }
})

export default function App() {
  const snap = useSnapshot(derived3)
  return (
    <div>
      <code>{snap.value}</code>
      <br/>
      <button onClick={() => ++state.value}>+1</button>
    </div>
  );
}
dai-shi commented 2 years ago

good point. no, derive is not designed for dataflow graphs. it's async push. (btw, jotai should work well for it. it's pull.) I'm not sure if we could achieve the "glitch-free" behavior because of the async batching behavior. That said, it's worth trying and i'm fairly interested. We'd do it as a third-party library at least for a start.

ziriax commented 2 years ago

Thanks for the quick reply, and reference to jotai.

Indeed, sorry, I should have seen this is not a dataflow approach: as no dependencies are passed to derive, it would be hard to figure out the graph at construction time. If a dependency array would be passed to derive, just as in React hooks, it might be possible to figure out the dependencies.

But if this library is designed for async push like ReactiveX, then I guess it is by design anyway?

dai-shi commented 2 years ago

I guess the dataflow graph can be constructed on the first run (and re-constructed on every run). Async push could be a hurdle. Is ReactiveX the same, non glitch-free?

Anyway, that reminds me. Can you do experiments with proxyWithComputed instead of derive? It's pull based.

ziriax commented 2 years ago

As far as I recall, ReactiveX is full of glitches, you have to take manual actions to avoid these. Most people don't care. It is also slow when the same observable is used in many different places in the graph, e.g. if you would make a videogame and pass the current time as a signal for animation, you would get a lot of redundant callback invocations when combining multiple inputs.

I'll experiment with proxyWithComputed, thanks.

dsacramone commented 2 years ago

As far as I recall, ReactiveX is full of glitches, you have to take manual actions to avoid these. Most people don't care. It is also slow when the same observable is used in many different places in the graph, e.g. if you would make a videogame and pass the current time as a signal for animation, you would get a lot of redundant callback invocations when combining multiple inputs.

I'll experiment with proxyWithComputed, thanks.

Did you get a chance to mess with proxyWithComputed?

ziriax commented 2 years ago

Amazing!

Noitidart commented 2 years ago

Thanks for this, so proxyWithComputed is glitch-free?

Noitidart commented 2 years ago

I just tested it, proxyWithComputed is glitch-free - https://codesandbox.io/s/proxywithcomputred-9bqfx - the callback only fires once

And the derive with sync false is glitched - https://codesandbox.io/s/dervie-glitch-0ji5r

dai-shi commented 2 years ago

@Noitidart Can you investigate why the test is passing? https://github.com/pmndrs/valtio/blob/2ea958bf9af99f205de718ff85b9c2c5eb4a31ce/tests/derive.test.tsx#L304 The sync must be off by default.

Noitidart commented 2 years ago

@Noitidart Can you investigate why the test is passing?

https://github.com/pmndrs/valtio/blob/2ea958bf9af99f205de718ff85b9c2c5eb4a31ce/tests/derive.test.tsx#L304

The sync must be off by default.

Hey thanks I looked into it. sync is true by default now right? That's why it's passing I think.

dai-shi commented 2 years ago

Isn’t sync false by default?

Noitidart commented 2 years ago

Oooh I thought it changed in the new version to default to true. Yeah thats curious I'll take a look!

Noitidart commented 2 years ago

Oh wow so something is broken. It's glitched in old version of v1.2.9 but in v1.2.10 it's no longer glitched, but its bugged.

Here is codesandbox. After incrementing button, state.value goes to 1, but derived1.value and derived2.value stay 0.

Here is codesandbox with v1.2.9 - https://codesandbox.io/s/dervie-glitch-0ji5r - if you increment it will change all to 1 but with 3 total callbacks, and 2 total render.

If you change that valtio to v1.2.10 you will see the derived values don't update.

fixed test - https://github.com/pmndrs/valtio/pull/341

dai-shi commented 2 years ago

Thanks for looking into it!

Noitidart commented 2 years ago

Thanks! :D