-
-
Notifications
You must be signed in to change notification settings - Fork 264
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
Comments
@drcmda Yeah, I noticed this limitation and knew there's no way to fix/workaround this without introducing a new API.
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.) In summary: a) opt-out scenario:
b) opt-in scenario:
I thought this lib should support classes as first-class, and took a) approach. I would like to hear your opinion. |
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()) |
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. |
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. |
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 scenariothe default behavior is to wrap any objects with proxies as much as possible. (there are some unsupported objects like 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 pros: class state works flawlessly. class MyState {
count = 0
increment() { ++this.count }
}
const state = proxy({
myState: new MyState(),
otherObj: {},
}) b) opt-in scenariothe 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 pros: objects with prototype are not wrapped with proxies, and just treated as a ref (like unsupported import { proxy } from 'valtio'
const state = proxy({
count: 0,
obj: {},
node: document.body, // this is just a ref by default
}) NotesThe 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. |
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. |
@drcmda Thanks for the convincing example. Huge json data was my concern too. Let's go with a). |
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 |
Would it be possible to add some docs on how // 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 |
@shamilovtim I think |
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.
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.
The text was updated successfully, but these errors were encountered: