pmndrs / valtio

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

Possible to wrap snapshot and state into one? #38

Closed steve8708 closed 3 years ago

steve8708 commented 4 years ago

With JSX Lite we hope to support Valtio, but there is one outstanding issue now that I understand the separation between state and snapshots which is that we don't always know at compilation time if a read or write will happen. This is because references can be grabbed and passed to functions as arguments, and the functions may do reads and writes on objects that we don't know at compile time what they trace back to

So if we could just have one object returned that can be read and written to, that would resolve this challenge . For instance this could be done with the help of a couple libraries like on-change and lodash, in theory, roughly like the below

import { useLocaProxy } from 'valtio/utils'
import { useRef } from 'react;
import onChange from 'on-change';
import { set } from 'lodash';

/**
 * @example
 *    const state = useMustableProxy(() => ({ name: 'Steve' }))
 *    return <input value={state.name} onChange={e => state.name = e.target.value }>
 */
const useMutableProxy = <T extends object>(init: T | (() => T)) => {
  const [snapshot, proxy ] = useLocalProxy(init) 
  const ref = useRef();
  if (!ref.current) { 
    ref.current = onChange(snapshot, (path, value) => set(state, path, value));
  }  

  return ref.current;
}

but if Valtio supported this, e.g. as an optional util, that would allow us to support it properly and be easy for those coming from mobx etc (or like the elegance of not having to think of state vs snapshots and just treat the proxy like a plain mutable object)

steve8708 commented 4 years ago

Just saw your reply on twitter to this @dai-shi and makes sense. This may make supporting Valtio tough for JSX Lite or not possible sadly, but will keep thinking on and open to any ideas you have here (if any)

dai-shi commented 4 years ago

A bit of context: Valtio as well as my another project react-tracked uses what I call state usage tracking.

For example:

const state = proxy({ a: 1, b: 2 })

const Component = () => {
  const snapshot = useProxy(state)
  return <div>{snapshot.a}</div>
}

This component only re-renders if a is changed. state.b++ doesn't trigger re-render. Technically, it is a bit different from MobX React observer HoC approach. Our approach requires the snapshot to be immutable (because we don't know when render ends.)

Now, I first thought your idea of on-change would work, if we can accept the confusing behavior shown in https://twitter.com/dai_shi/status/1333620437792616449

However:

const combined = onChange(snapshot, (path) => set(snapshot, path, state))
combined.a // tracked 'a' usage
combined.b = 3 // this is also tracked as 'b' used

So, the component will re-render if state.b is changed, even if it doesn't display it. This is true if we do state.b = snapshot.b + 1, so this is a pitfall of this lib.

Again, the confusing behavior is:

combined.b++ // internally state.b becomes 3
combined.b++ // the state.b is still 3
console.log(combined.b) // will show 2...

There might be a better way that I don't consider now. The current behavior requires user to understand the difference between immutable snapshot for read and mutable state for write.

This may make supporting Valtio tough for JSX Lite or not possible sadly

That's unfortunate. (btw, I like the idea of JSX Lite very much.)

but will keep thinking on and open to any ideas you have here (if any)

Let's keep this issue open for a while and see if anyone is interested in investigation.

steve8708 commented 4 years ago

Thanks for the detailed info @dai-shi - really appreciated. All makes a lot of sense here. I'll continue to stew on if we can make this work.

I also realized I could also better illustrate our challenge as well if you have any ideas

import { useState } from '@jsx-lite/core'

export default function MyJSXLiteComponent(props) {
  const state = useState({
     getProductName(product) {
        // Needs to be snapshot, but we don't really know what `product` is at compile time to make that swap
       return product.name;
     },
     setProductName(product, name) {
       // Needs to be `state`, but we don't really know what `product` is here at compile time either
       product.name = name
     }
  })

  return (
    <div onClick={() => {
      // Right here, to the compiler, this appears like it should be using `snapshot` bc it's accessing a value, but that's wrong as it's only to pass to a helper to mutate the value, so it needs to be using `state`
      state.setProductName(state.products[0], 'foobar')
    }>
       {state.getProductName(state.products[0])}
   </div>)
}
dai-shi commented 4 years ago
      // Right here, to the compiler, this appears like it should be using `snapshot` bc it's accessing a value, but that's wrong as it's only to pass to a helper to mutate the value, so it needs to be using `state`
      state.setProductName(state.products[0], 'foobar')

This is fine. We should just read from state in callbacks / effects. We should only read from snapshot in render.

(I'm still not sure if I fully understand the challenge, though. As I'm interested in your compiler approach, I'd like to take a closer look when I got chance.)

thelinuxlich commented 3 years ago

Since useLocalProxy is available in 0.4.5, maybe we can close this issue?

dai-shi commented 3 years ago

Well, this is a discussion on top of useLocalProxy. Anyway, the original question is answered, so let's close this. We can continue discussing alternative ideas here, or create a new issue.