pmndrs / valtio

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

A computed field deriving values from other computeds doesn't work #82

Closed thelinuxlich closed 3 years ago

thelinuxlich commented 3 years ago

The dependency track fails when a computed needs the value of another computed.

dai-shi commented 3 years ago

Would you share some code snippets? I'm not sure if I understand the point.


If I understand correctly, this is expected. The dependency tracking only works within the state tree.

Now, I'd like to emphasize again that dependency tracking is not first-class in valtio. The goal of valtio is to provide a mechanism to create immutable snapshots from mutable state. This is modeled after useMutableSource and it would be probably necessary to work well in concurrent mode. The design choice was to support usage tracking in render, rather than dependency tracking. This provides unique useProxy hook. (A side note: there are non-technical reasons. this is to ease the implementation by sharing implementation with react-tracked and to avoid competing with mobx.) So, valtio is not comparable with mobx in its capability. It's more comparable with immer.

So, #71 was to add a new layer on top of valtio core to do dependency tracking at its best and the easiest. As I said, I would never add it to the core because it's not the goal and dependency tracking and usage tracking are competing theoretically.

Having that said, the suggestion of this issue is actually interesting to me. One of my hesitations with #71 is it doesn't add any new semantics. It was only about developer expectations and performance in some rare cases. Performance is unknown. It may perform better or worse. Again this is because dependency tracking and usage tracking is competing. The reason I'm interested in this issue is it would add a new feature not just about performance.

But we'd need to add a new semantics. We can't do it magically like mobx. I haven't considered further and this is just a rough thought:

const state = proxyWithComputed({
  countA: 0,
}, {
  sum: {
    get: (snapFromThis, snapFromAnotherState) => snapFromThis.countA + snapFromAnotherState.countB,
    externals: [anotherState],
  },
})

This is challenging and exciting to add a full dependency tracking feature on top of valtio core. I'm not really sure if this is good DX-wise and worth trying...

thelinuxlich commented 3 years ago

This API would be too clumsy. I'm thinking about options....

dai-shi commented 3 years ago

Hm, I think I misunderstood.

const state = proxyWithComputed({
  count: 0,
}, {
  doubled: (snap) => snap.count * 2,
  quadrupled: (snap) => snap.doubled * 2,
})

Do you mean something like this?

thelinuxlich commented 3 years ago

Exactly!

dai-shi commented 3 years ago

Yeah, it's not supported with the current version. Would be nice to fix. Wonder how easy it is.

What would/should happen if there was mutual dependency?


const state = proxyWithComputed({
  count: 0,
}, {
  foo: (snap) => snap.count + snap.bar * 2,
  bar: (snap) => snap.count + snap.foo / 2,
})
thelinuxlich commented 3 years ago

In my opinion, to solve the circular dependency we have two options: the first value of a computed field before the first derivation should be null OR just throw a error in this particular use case

thelinuxlich commented 3 years ago

I'm more inclined to throwing a error because without a initial value this looks like a infinite loop.

thelinuxlich commented 3 years ago

@ryansolid please give us an opinion here :)

ryansolid commented 3 years ago

That example can never resolve. It has to error. Simple atomic primitives that eager evaluate will almost never get in this state because order of variable declarations. But libraries that offer this sort of proxy augmentation need to detect this. MobX for example: https://codesandbox.io/s/inspiring-dirac-zx922?file=/src/index.js

To be fair this problem isn't limited to reactive libraries. Using pure getters would cause that to maximum call stack anyway. So while it's nice MobX detects it, it would always fail from JavaScript semantics. Here it is part of the API so may not be as obvious, but an error none the less.

dai-shi commented 3 years ago

Implementation-wise, I got another concern.

const state = proxyWithComputed({
  count: 0,
}, {
  doubled: (snap) => snap.count * 2,
  quadrupled: (snap) => snap.doubled * 2,
})

We need to evaluate doubled before quadrupled, but the current emulation is hard for it as each is evaluated individually. Hm, I'm not even sure why the current impl works. Maybe, it depends on the Reflect.ownKeys() order.

https://stackoverflow.com/questions/5525795/does-javascript-guarantee-object-property-order/38218582#38218582

@thelinuxlich Do you know if this

}, {
  doubled: (snap) => snap.count * 2,
  quadrupled: (snap) => snap.doubled * 2,
})

works, but this

}, {
  quadrupled: (snap) => snap.doubled * 2,
  doubled: (snap) => snap.count * 2,
})

doesn't work?

thelinuxlich commented 3 years ago

Yeah, the first one works, so order matters.

thelinuxlich commented 3 years ago

This should be closed and we need to update the docs with the caveat of computed order to have these dependencies.

dai-shi commented 3 years ago

Thanks for the confirmation. I got the clear understanding of how it's working currently: It relies on Reflect.ownKeys() ordering in core. Not only for proxyWithComputed, but for all functions and getters in proxy object. Good to know.

Now, I spent some time to think about if we could fix it. This is not a trivial thing. We could hack only for proxyWithComputed, but it makes less performant. So, I'm happy with your suggestion. The caveat in docs should work the best for now.

thelinuxlich commented 3 years ago

Yeah it's minor, considering a similar reactive library(React Easy State) kinda does the same thing:

import { store } from '@risingstack/react-easy-state';

const store1 = store({ name: 'Store 1' })
const store2 = store({ get name () { return store1.name } }) // you need to create a separate store for computeds
ryansolid commented 3 years ago

MobX gets around this by using lazy evaluation and a system based on push/pull. It's much more complicated. In that way they don't execute the computations until after the proxy object has already been fully initialized (ie. on first read) so order doesn't matter.

dai-shi commented 3 years ago
const store1 = store({ name: 'Store 1' })
const store2 = store({ get name () { return store1.name } }) // you need to create a separate store for computeds

By the way, this is something I misunderstood in my first comment. This pattern of depending a separate store is not possible in valtio.

thelinuxlich commented 3 years ago

@dai-shi yeah I remember you saying something about it when we were brainstorming the computed feature, I prefer Valtio syntax :)

thelinuxlich commented 3 years ago

@ryansolid I think Vue.js doesn't need a order too, by the way @dai-shi both mobX and Vue.js(and probably many other reactivity libraries) have some caching strategy on computeds to avoid repeating expensive operations. Might think about it in Valtio too!

thelinuxlich commented 3 years ago

@ryansolid btw Valtio has a vanilla version, would it be hard for me to use it with Solid?

dai-shi commented 3 years ago

caching strategy on computeds to avoid repeating expensive operations.

proxyWithComputed does have a cache. It's almost like homegrown version of proxy-memoize. But, again I wouldn't add the current implementation to the core, because for the react version of valtio, this feature is overlapping. If we could come up with more efficient implementation, we might. (There's a possibility as you originally suggested somewhere. It's not trivial, we need to track property mutation. That's gonna be v2.)

thelinuxlich commented 3 years ago

Oh you mean storing the dependencies in the state as a Map of Symbol, property

thelinuxlich commented 3 years ago

Why do you think this feature overlaps with React?

ryansolid commented 3 years ago

Yes Vue works that way too.

As for vanilla usage it's a subscribable so it wouldn't be that hard on the surface, like working with RxJS. But I think we would lose of the benefits over translation. Because the granularity of updates is only going to be as far as Valtio notifies. So like in React you basically tell the component to re-render when you know that it listens to something on your proxy. Even Svelte is basically the same.

Solid's whole renderer is like the proxy right down to the JSX. So it would be inefficient to tell the component to re-render (there is actually no way to do this). We'd want to tell the exact binding to re-render and that seems like a lot of manual connection work. That's why Solid has auto tracking like Vue/MobX because I can apply that at the binding expression level and not need to manually wire up subscriptions. Even props and prop expressions get passed through without being tracked by the intermediate components.

So means that most 3rd party state solutions are less performant than our own primitives since we tend to need to diff their output before applying it. There are benefits to the features, tooling and philosophy API but similar looking solutions probably have less motivation to exist there (like you won't find MobX or Vue reactivity in Solid) since the internal proxies are so much better suited. A simple wrapper to ape the other libraries API gets very similar effect but much better results performance wise.

Unfortunately that doesn't always encourage existing devs to the platform. It really is that different though.

dai-shi commented 3 years ago

Why do you think this feature overlaps with React?

Let's see a simple example:

const state = proxy({
  text: 'hello',
  count: 0,
  getDoubled() { return this.count * 2 },
})

const Counter = () => {
  const snap = useProxy(state)
  return <div>{snap.getDoubled()}</div>
}

When we change text like state.text = 'hello2', the Counter component will not re-render. This render optimization is super powerful, and the main goal of valtio react.

Now, if we use object getters (note this is undocumented),

const state = proxy({
  text: 'hello',
  count: 0,
  get doubled() { return this.count * 2 },
})

const Counter = () => {
  const snap = useProxy(state)
  return <div>{snap.doubled}</div>
}

This is still fine. It doesn't re-render, because doubled value is not changed.

So, using proxyWithComputed doesn't help optimizing re-renders at all. It only eliminates invoking the getter function, with the cost of proxy-memoize like tracking. And the latter cost can be bigger than the former cost.

thelinuxlich commented 3 years ago

In a simple example, yes, you are right. In a real world use case where the state object might span 100 properties and you might have 100 computeds, firing the 100 computeds whenever any of the 100 properties change is not optimal.