fix(vanilla): proxy can be created from snapshot #673
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit 992c2bf:
|
|
Size Change: -7 B (0%) Total Size: 56.1 kB
ℹ️ View Unchanged
|
|
Looks good to me, and that does the trick.
As alternative, I would suggest to just have a way to convert the snapshot back to POJO (recursively). Is there any way to get this done? Maybe I missed something, again. Like, MobX has this |
|
I said not intended, but I think it's valid. What you pass to
Do you mean a snapshot to a new plain object, that's basically deep cloning. ex. |
Yep, so that I can use it to create a new proxy. If the current way isn't correct (but I see that you said it just not intended). In my case, I don't really care much of what happens with nested objects, just needed to put part of the state to context for easy access from within its descendants. |
When you create a new proxy, it will copy the
My question, in your use case, is if you can pass the child proxy via context, instead of creating a new proxy (which is isolated from the original proxy. maybe it's intentional.) |
Is this valid? I don't quite understand, maybe. Documentation says that I need to use snapshots within render function. Does the component react on changes in state object? |
|
It's valid, but very tricky, so it's hard to educate. const Component = () => {
useSnapshot(state.child) // use it here to subscribe
return (
<MyContext.Provider value={state.child}>
...
We've changed the word over time, maybe it's still not ideal. |
| const state = proxy({ c: 0 }) | ||
| const snap1 = snapshot(state) | ||
| const state2 = proxy(snap1) | ||
| expect(state2.c).toBe(0) |
There was a problem hiding this comment.
@dai-shi with this approach, this assertion fails:
expect(state2).toBe(state);
Wdyt of teaching proxy to realize "I'm being passed a snapshot" and then using some sort of getOriginalObject / getUntracked approach to just recover the existing proxy?
It seems like it'd be nice to get back the same store identity.
Or, alternatively, just throw an error and say "you passed a snapshot to proxy...don't do that".
There was a problem hiding this comment.
Or, alternatively, just throw an error and say "you passed a snapshot to proxy...don't do that".
Wouldn't this be a breaking change?
There was a problem hiding this comment.
Yes, I'm just thinking if @dai-shi thinks the "proxying a snapshot" is enough of a unintended use case / probably not what the user really wanted to do, it'd be better to just throw and guide them to a better pattern, i.e. specifically passing the store around directly (which I believe is the recommended approach).
(Or, maybe we do the 1st option, which is that "proxying the snapshot" does become the canonical way of getting "back to the store" from the snapshot, which again AFAIU is unintended/accidental behavior today.)
There was a problem hiding this comment.
Maybe this the misguided. I think we should support creating a new proxy from a frozen (incl. non-configurable&non-writable) object. And, a snap is just an example of it. And we support it.
It's definitely not about recovering the original proxy.
There was a problem hiding this comment.
I think we should support creating a new proxy from a frozen
Ah sure; is that what @octet-stream wants to do? I might have misread the thread, but I thought the idea was to get back to the original store.
I guess Nick's comment "so that I can use it (the snapshot) to create a new proxy" does explicitly ask for going from snapshot --> "new store", but the intervening comments made me jump to the conclusion Nick was running into the "how do I get a store in a child that was passed a snapshot" wrinkle.
And not literally "I want a completely separate / 2nd additional store that is 100% detached from the first store".
But I could have made the wrong assumption.
There was a problem hiding this comment.
From @octet-stream 's example, he almost certainly wants the original store; he's using a useSnapshot against a root list of notes:
const {items} = useNotesStateSnapshot()
return (
<ul>
{items.map((note) => (
<li key={note.id}>
<NoteStateContextProvider data={note}>
<NoteCard />
</NoteStateContextProvider>
</li>
))}
</ul>
Passing each note (a compare proxy) into a child context and doing proxy(data):
const StateContextProvider: FC<ProviderProps<T>> = ({data, children}) => {
// Use `useMemo` instead of `useRef` with `valtio`, so that we can reload page state if the `data` is changed
const state = useMemo(() => proxy(data), [data])
Which I just really have to assume the desire is "get me the note[0] model from the original store, so I can mutate it in my child if I want":
This is why I think calling proxy(snapshotFromSnapshot) or proxy(snapshotProxyFromUseSnapshot) should either:
a) throw a "this is probably not what you meant to do" un-intended use error, or
b) return the original underlying store that creates the snapshot snapshot or `useSnapshot
Instead of the current behavior which, AFAIU creates a 2nd store that is 100% disconnected (different data, different subscribers) from the 1st store and just surely is not what the user wanted.
There was a problem hiding this comment.
he almost certainly wants the original store
It might be the case, and if so, we could do it right? #657 (comment)
There was a problem hiding this comment.
Yes. @octet-stream can you confirm if @dai-shi 's linked comment is what you're trying to do?
Fwiw @dai-shi this is what the docs PR I opened tries to clarify for users of useSnapshot; it has your example as-is, explaining that they should pass the store if they want to mutate.
I don't think I included a disclaimer that if they proxy a snapshot, it will make a completely separate store, but that's probably a good idea.
Fwiw I wrote up my current understanding of the behavior / your recommendation's @dai-shi here: Happy to edit if I misunderstood. |
@octet-stream per this ask of "just give me a POJO", FWIW this is what However, if you're using If you have a provided-by- Granted, the user should know "don't render against this", b/c they will lose access tracking, but if they wanted to put the snapshot on the wire somewhere, they probably would prefer using the Granted, @dai-shi perhaps your suggestion is for the user to just always have the original store available, and they can Basically given either of a |
|
I feel like allowing snap -> state conversion leads to misconception, but I might be wrong.
you get it right. I'd suggest to avoid overusing what (basically, we want to make the api surface as small as possible.) |
I believe I said before that I don't really care if that would be original store, or a separate one. For me, creating a new store from snapshot is just a natural way to pass the store's parts to through Within the |
|
Also, the main point is still that you broke previous behaviour in minor update (or even patch update?). I almost certain it still falls into breaking changes category, if you follow semver. |
Related Issues or Discussions
Fixes #670
Summary
#654 broke one use case described in #670. (tbh, this isn't intended use case).
When copying from an initial object to a proxy base object, we should make it writable to make the proxy object mutable.
Check List
yarn run prettierfor formatting code and docs