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

Possible to wrap snapshot and state into one? #38

Closed
steve8708 opened this issue Dec 1, 2020 · 6 comments
Closed

Possible to wrap snapshot and state into one? #38

steve8708 opened this issue Dec 1, 2020 · 6 comments

Comments

@steve8708
Copy link
Contributor

steve8708 commented Dec 1, 2020

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

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

dai-shi commented Dec 1, 2020

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

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

dai-shi commented Dec 1, 2020

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

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

@dai-shi
Copy link
Member

dai-shi commented Dec 3, 2020

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.

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

No branches or pull requests

3 participants