pmndrs / valtio

🧙 Valtio makes proxy-state simple for React and Vanilla
https://valtio.dev
MIT License
8.84k stars 248 forks source link

feat: Add isStore. #675

Closed stephenh closed 1 year ago

stephenh commented 1 year ago

Related Issues or Discussions

Related to my useStore prototypes (https://github.com/pmndrs/valtio/discussions/662)

Summary

When working generically with a store, and wanting to handle it recursively, it's useful to ask "is this nested value itself a store?".

Currently this is not necessary for Valtio & proxy-compare, b/c createSnapshot pushes "this is special"-ness into proxy-compare's markToTrack, but 3rd party libraries/hooks like useStore don't have that blessed push-based mechanism, so need a pull-/probed-based mechanism to detect store-ness/special-ness.

Very open to alternative approaches/impls that accomplish the "is this a store?" goal.

Check List

vercel[bot] commented 1 year ago

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 19, 2023 at 5:14PM (UTC)
codesandbox-ci[bot] commented 1 year ago

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 d128f088f12d58b2e1c25a40fd5637d5347021b0:

Sandbox Source
React Configuration
React Typescript Configuration
React Browserify Configuration
React Snowpack Configuration
React Parcel Configuration
dai-shi commented 1 year ago

isStore should be implemented on your end. Something like this:

const isStore = (x) => getVersion(x) !== undefined;

There might be room for improvement in docs.

I think I did it in some of my valtio third-party libraries.

Closing.

dai-shi commented 1 year ago

There might be room for improvement in docs.

Yeah, we should add this isStore recipe in: https://github.com/pmndrs/valtio/blob/main/docs/api/hacks/getVersion.mdx

Another impl:

const isStore = (x) => typeof getVersion(x) !== 'number'`;

BTW, I prefer isProxy to isStore.

stephenh commented 1 year ago

Ah darn it, I thought proxyStateMap was per proxyFunction but you're right it's global.

That makes sense! I forgot about getVersion.

Cool, I'll do that.

BTW, I prefer isProxy to isStore.

Yeah, my only nudge is that once we have ~2-3 types of proxies floating around, it can be hard to keep track of "which proxy is this again now?". :-)

But that's fine.

stephenh commented 1 year ago

Yeah, we should add this isStore recipe in:

Wdyt about adding isProxy but based on your impl, i.e. a helper method that just calls getVersion?

Just thinking it's a more ergonomic/more discoverable to users than having to find it as a suggested recipe in the getVersion docs and then implement their own.

dai-shi commented 1 year ago

It is only for library authors. I'd prefer hiding it in the getVersion docs.

stephenh commented 1 year ago

Sounds good. Fwiw just in terms of push-vs-pull, it might be neat if instead of markToTrack-ing every snapshot, Valtio just did like:

markToTrackFunction((obj) => isSnapshot(obj);

Which would let proxy-compare ask "should I proxy this object?" by just calling the function.

In theory it'd cut down on the overhead of pushing every snapshot into the mark to track set. :shrug:

Not a big deal, just mentioning as a potential micro-optimization.

dai-shi commented 1 year ago

Yeah, that would be another way. For now, I avoided it because it's not predictable in terms of performance. People can register a heavy function, and multiple functions (which requires to iterate those functions). But, let me keep that in mind for the future version of proxy-compare, which I want to simplify further (your suggestion of not supporting frozen object might be in.)

stephenh commented 1 year ago

People can register a heavy function

Good point!