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

💊 [feature request]: valtio should support non-serializable data, too #61

Closed
drcmda opened this issue Jan 3, 2021 · 10 comments · Fixed by #62
Closed

💊 [feature request]: valtio should support non-serializable data, too #61

drcmda opened this issue Jan 3, 2021 · 10 comments · Fixed by #62

Comments

@drcmda
Copy link
Member

drcmda commented Jan 3, 2021

Valtio needs something that allows users to put non proxied data into the store. Currently it cannot handle foreign classes and objects, since accessors to parents/children end up proxying the entire system because Valtio cannot differentiate and just climbs through the nested graph. This is shaping up to be a bigger concern because right now it can only hold serializable data.

A suggestion would be to introduce some kind of hint that makes it only check reference equality if the users wants it so.

import { proxy, ref } from 'valtio'

const state = proxy({
  status: true,
  city: 'Berlin',
  body: ref(document.body), // < will not be proxied
  audio: ref(new Audio('/hello.mp3')), // < will not be proxied
})

// I guess it needs to be consistent, so overwrites probably need it, too ...
<div onClick={() => state.audio = ref(new Audo('/chime.mp3'))} />

This wouldn't work with SSR or localCache, but at least we have the ability to throw complex foreign state into the store in local scenarios.

@dai-shi
Copy link
Member

dai-shi commented Jan 3, 2021

@drcmda Yeah, I noticed this limitation and knew there's no way to fix/workaround this without introducing a new API.

ref sounds pretty nice and implementation-wise it's trivial. I can work on that. I need your help on readme, most likely.

Before moving forward, I would like to suggest another possibility (this is a breaking change, but we should do it before v1 if we were to do it.)
ref idea is to opt-out proxying, but we could do the other way around. opt-in.
This works only for classes or any object that have a custom prototype.
In this case, by default valtio would only wrap plain objects and arrays. And all other objects are treated as a reference.

In summary:

a) opt-out scenario:

  • all objects are wrapped by default unless listed as unsupported.
  • mark any objects with ref to opt-out wrapping.

b) opt-in scenario:

  • all "plain" objects and arrays are wrapped by default
  • class-based objects or objects with prototype has to be marked somehow (a special symbol property?) to opt-in wrapping.
  • there's no way to opt-out plain objects/arrays.

I thought this lib should support classes as first-class, and took a) approach.
Now, given this discussion, I think b) would be a considerable option if we'd anyway document explicitly how to use classes in valtio.

I would like to hear your opinion.

@dai-shi
Copy link
Member

dai-shi commented Jan 3, 2021

class-based objects or objects with prototype has to be marked somehow

For classes, something like this would seem natural.

import { proxyClass } from 'valtio'

@proxyClass
class MyState {
  count = 1
  increment() { ++this.count }
}

const state = proxy(new MyState())

@Aslemammad
Copy link
Member

I think b is a good option. BTW i love the decorator syntax, but I think some people would have problem with it because of babel things.

@drcmda
Copy link
Member Author

drcmda commented Jan 3, 2021

i think opt out is better in this case imo. decorators are pretty terrible, with classes being less and less relevant i've never seen them around much. most usecases for valtio are probably proxy, that's why people use it in the first place. for the few non-serializable variables i'd prefer a way out without changing valtio fundamentals.

@dai-shi
Copy link
Member

dai-shi commented Jan 3, 2021

Okay, opt-out is totally fine with me. For opt-in, Let's forget about decorators for now, as I'm not a big fan either, tbh.

I'd like to rephrase again about pros and cons of opt-out and opt-in solutions.

a) opt-out scenario

the default behavior is to wrap any objects with proxies as much as possible. (there are some unsupported objects like Set.)

if you want to avoid wrapping, you opt-out.

import { proxy, ref } from 'valtio'

const state = proxy({
  count: 0,
  obj: {},
  node: ref(document.body),
})

cons: you need to opt-out with ref.

pros: class state works flawlessly.

class MyState {
  count = 0
  increment() { ++this.count }
}

const state = proxy({
  myState: new MyState(),
  otherObj: {},
})

b) opt-in scenario

the default behavior is to wrap only plain objects (incl. arrays) with proxies. this doesn't apply to objects that have custom prototype.

cons: you need to opt-in class defined objects to wrap with proxies.

import { proxy, wrap } from 'valtio'

const state = proxy({
  myState: wrap(new MyState()),
  otherObj: {},
})

(The exported function wrap wouldn't be named nicely. Would be better something else.)

pros: objects with prototype are not wrapped with proxies, and just treated as a ref (like unsupported Set.)

import { proxy } from 'valtio'

const state = proxy({
  count: 0,
  obj: {},
  node: document.body, // this is just a ref by default
})

Notes

The reason I wanted to make this clear is that the only reason I added the support to wrap objects with prototype is for class use case. If we don't need to support class based state without any tricks, things are much simpler. Now I heard about this issue, we want to re-consider if supporting class state by default is a good design.

@drcmda
Copy link
Member Author

drcmda commented Jan 3, 2021

as for b, i think the problem is generally that sometimes there may be data you don't want proxied period, no matter if it's a class, an object or an array. it seems to make assumptions now that are kind of magic or would be hard to explain. but it also wouldn't work: for instance, say the server sends a huge 20mb json package that i need to place into the state model. i just want valtio to hold it for me for convenience (otherwise i'd have to use a different store in parallel). with option b it would proxy it, because it's just an object. without a way to bail out.

in vue they're using option a as well for that reason, the "created" hook that allows you to do this.nonReactiveData = document.body.

@dai-shi
Copy link
Member

dai-shi commented Jan 4, 2021

@drcmda Thanks for the convincing example. Huge json data was my concern too. Let's go with a).

@Noitidart
Copy link
Contributor

Noitidart commented Sep 21, 2021

Loved your guys thought process in the open here! It's like I was apart of it haha but I'm just freeloading. Just use the ref api and it was beautiful.

@shamilovtim
Copy link

shamilovtim commented Nov 19, 2022

Would it be possible to add some docs on how ref() renders with useSnapshot? I'm confused because the docs describe it as checking referential equality. That would make sense but then I would expect the following to re-render on every single click of a button, but it does not.

// myStore.ts
const myStore = proxy({
  foo: ref(complexArray)
})

// MyScreen.tsx
const snap = useSnapshot(myStore).foo;
console.log('rendered myScreen');
const randomArr = [];

const onPress = () => {
  // this does not rerender MyScreen.tsx
  myStore.foo = ref(randomArr);
});

Update: I figured this out. It was just that the ref(randomArr) from the onPress was re-using the reference (and not creating a new one). I had to use a useState to force the reference to change in the callback and that confirmed that ref() does work correctly with useSnapshot when a reference changes

@Noitidart
Copy link
Contributor

@shamilovtim I think ref things are omitted by useSnapshot, as thats the use case, to ignore huge documents or etc.

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.

5 participants