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

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

Closed
ziriax opened this issue Dec 17, 2021 · 15 comments · Fixed by #335
Closed

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

ziriax opened this issue Dec 17, 2021 · 15 comments · Fixed by #335

Comments

@ziriax
Copy link

ziriax commented Dec 17, 2021

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

dai-shi commented Dec 17, 2021

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

ziriax commented Dec 17, 2021

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

dai-shi commented Dec 17, 2021

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

ziriax commented Dec 18, 2021

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

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

ziriax commented Jan 24, 2022

Amazing!

@Noitidart
Copy link
Contributor

Thanks for this, so proxyWithComputed is glitch-free?

@Noitidart
Copy link
Contributor

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

dai-shi commented Jan 24, 2022

@Noitidart Can you investigate why the test is passing?

it('basic (#296)', async () => {

The sync must be off by default.

@Noitidart
Copy link
Contributor

@Noitidart Can you investigate why the test is passing?

it('basic (#296)', async () => {

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

dai-shi commented Jan 25, 2022

Isn’t sync false by default?

@Noitidart
Copy link
Contributor

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

@Noitidart
Copy link
Contributor

Noitidart commented Jan 25, 2022

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 - #341

@dai-shi
Copy link
Member

dai-shi commented Jan 25, 2022

Thanks for looking into it!

@Noitidart
Copy link
Contributor

Thanks! :D

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

Successfully merging a pull request may close this issue.

4 participants