fix(vanilla): proxy can be created from snapshot

[dai-shi]

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

• [x] yarn run prettier for formatting code and docs

[vercel[bot]]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated
valtio	✅ Ready (Inspect)	Visit Preview	💬 Add your feedback	Feb 24, 2023 at 11:30PM (UTC)

[codesandbox-ci[bot]]

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 992c2bfc1d27d4207a39d57cb5038a18d7bf47b4:

Sandbox	Source
React	Configuration
React Typescript	Configuration
React Browserify	Configuration
React Snowpack	Configuration
React Parcel	Configuration

[github-actions[bot]]

Size Change: -7 B (0%)

Total Size: 56.1 kB

Filename	Size	Change
dist/esm/vanilla.js	2.41 kB	-7 B (0%)
dist/system/vanilla.development.js	2.55 kB	-7 B (0%)
dist/system/vanilla.production.js	1.49 kB	+7 B (0%)
dist/umd/vanilla.development.js	2.72 kB	-2 B (0%)
dist/umd/vanilla.production.js	1.61 kB	+4 B (0%)
dist/vanilla.js	2.6 kB	-2 B (0%)

ℹ️ View Unchanged

| Filename | Size | | :--- | :---: | | `dist/esm/index.js` | 62 B | | `dist/esm/macro.js` | 698 B | | `dist/esm/macro/vite.js` | 864 B | | `dist/esm/react.js` | 732 B | | `dist/esm/react/utils.js` | 221 B | | `dist/esm/utils.js` | 68 B | | `dist/esm/vanilla/utils.js` | 4.21 kB | | `dist/index.js` | 232 B | | `dist/macro.js` | 937 B | | `dist/macro/vite.js` | 1.09 kB | | `dist/react.js` | 668 B | | `dist/react/utils.js` | 237 B | | `dist/system/index.development.js` | 236 B | | `dist/system/index.production.js` | 170 B | | `dist/system/macro.development.js` | 779 B | | `dist/system/macro.production.js` | 556 B | | `dist/system/macro/vite.development.js` | 951 B | | `dist/system/macro/vite.production.js` | 660 B | | `dist/system/react.development.js` | 871 B | | `dist/system/react.production.js` | 471 B | | `dist/system/react/utils.development.js` | 316 B | | `dist/system/react/utils.production.js` | 221 B | | `dist/system/utils.development.js` | 241 B | | `dist/system/utils.production.js` | 176 B | | `dist/system/vanilla/utils.development.js` | 4.43 kB | | `dist/system/vanilla/utils.production.js` | 2.84 kB | | `dist/umd/index.development.js` | 372 B | | `dist/umd/index.production.js` | 322 B | | `dist/umd/macro.development.js` | 1.05 kB | | `dist/umd/macro.production.js` | 727 B | | `dist/umd/macro/vite.development.js` | 1.24 kB | | `dist/umd/macro/vite.production.js` | 883 B | | `dist/umd/react.development.js` | 814 B | | `dist/umd/react.production.js` | 526 B | | `dist/umd/react/utils.development.js` | 396 B | | `dist/umd/react/utils.production.js` | 297 B | | `dist/umd/utils.development.js` | 387 B | | `dist/umd/utils.production.js` | 335 B | | `dist/umd/vanilla/utils.development.js` | 4.7 kB | | `dist/umd/vanilla/utils.production.js` | 2.92 kB | | `dist/utils.js` | 236 B | | `dist/vanilla/utils.js` | 4.54 kB |

_{compressed-size-action}

[octet-stream]

Looks good to me, and that does the trick.

tbh, this isn't intended use case

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 .toJS utility that does exactly that.

[dai-shi]

I said not intended, but I think it's valid.

What you pass to proxy() is an object to initialize the proxy object. It's basically used and thrown away (but we keep the reference, which is tricky. I might re-consider it in the future version.)

a way to convert the snapshot back to POJO (recursively).

Do you mean a snapshot to a new plain object, that's basically deep cloning. ex. lodash.cloneDeep().

[octet-stream]

Do you mean a snapshot to a new plain object, that's basically deep cloning. ex. lodash.cloneDeep().

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.

[dai-shi]

Do you mean a snapshot to a new plain object, that's basically deep cloning. ex. lodash.cloneDeep().

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).

When you create a new proxy, it will copy the initialObject recursively anyway, so this PR would be better as of now (instead of cloning on your end.) So, it is valid.

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.

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.)

[octet-stream]

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?

[dai-shi]

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}>
      ...

I need to use snapshots within render function.

We've changed the word over time, maybe it's still not ideal. snapshot objects should be used in a component render function body, but if you pass things to child components (via props or contexts), they should be primitive values or state objects, not snapshot objects.

[stephenh]

snapshot objects should be used in a component render function body, but if you pass things to child components (via props or contexts), they should be primitive values or state objects, not snapshot objects.

Fwiw I wrote up my current understanding of the behavior / your recommendation's @dai-shi here:

https://github.com/pmndrs/valtio/pull/676

Happy to edit if I misunderstood.

[stephenh]

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?

@octet-stream per this ask of "just give me a POJO", FWIW this is what snapshot returns. Just a 100% POJO/disconnected copy of the store's data.

However, if you're using useSnapshot, then those are also "snapshots" but wrapped in a proxy.

If you have a provided-by-useSnapshot snapshot-proxy, IMO @dai-shi it would be desireable to be able to pass that to snapshot and basically "unwrap it" back to the non-access tracking / pure-POJO snapshot.

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 snapshot-based POJO and not useSnaphot-based POJO proxy.

Granted, @dai-shi perhaps your suggestion is for the user to just always have the original store available, and they can snapshot it directly if they want a POJO, instead of trying to recover snapshots (or stores per the comment from) from snapshot-proxies.

Basically given either of a snapshot snapshot a useSnapshot snapshot-proxy, I think the user should be able to easily "get back" to both the snapshot snapshot (from the snapshot-proxy by calling snapshot on it) as well as the non-snapshot store (from either the snapshot-proxy or the snapshot-pojo by calling proxy on it).

[dai-shi]

I feel like allowing snap -> state conversion leads to misconception, but I might be wrong.

perhaps your suggestion is for the user to just always have the original store available

you get it right. I'd suggest to avoid overusing what useSnapshot returns.

(basically, we want to make the api surface as small as possible.)

[octet-stream]

From @octet-stream 's example, he almost certainly wants the original store; he's using a useSnapshot against a root list of notes

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 React.Context to child components, and at the same time subscribe to the original store to track changes for the whole list (in this case).

Within the NoteStateContextProvider I can access a note that was passed to it. The child components will react to its changes anyway, so it's ok as long as it work that way. At the same time I could also update the original store, if needed, because all Note-related components also wrapped within NotesStateContextProvider, and all subscribed components will react to this change. Here, for example, I update the list when a new note is added. Because I subscribed to the original store, notes list will be updated as I expect, and for the new note the new NoteStateContextProvided with corresponding store is created. But then again, I don't care if that would be a separate store, or a part of the original.

[octet-stream]

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.