feat: Cache class getter calls.

stephenh commented 1 year ago

Summary

This makes the behavior the same as object getters, which I believe is both:

desirable in general (caching class getters as a feature)
reduces the complexity for users of having some getters work one way, and others work another; this is a more consistent mental model that "snapshots always cache getters".

Check List

[x] yarn run prettier for formatting code and docs

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 4, 2023 at 9:48PM (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 fcbfa2c33de558ee68e73420fb0d3f6a33949f29:

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

dai-shi commented 1 year ago

No, I can't stand adding complexity in vanilla and size increase. Please consider adding a util in valtio/utils to copy prototype to an initial object, if such thing is possible.

stephenh commented 1 year ago

No, I can't stand adding complexity in vanilla and size increase.

Hm, yeah, I sympathize with the goal, but will agree-to-disagree that this is a meaningful increase in complexity; if anything my goal is actually complexity reduction (...albeit to users).

Please consider adding a util in valtio/utils to copy prototype to an initial object , if such thing is possible.

Do you mean like:

const state = proxy(flattenPrototype(new Counter());

That copies the getters from the prototype down onto the instance?

I can see how that would work, so I'm good with that as an implementation approach, and will give it a try.

Would you then be open to proxy basically detecting target prototype !== object property and invoking flattenPrototype for callers?

My concern is having to explain to users the semantic difference between object getters and class getters, such that if we can find a way for both of them to work semantically the same (and why not cache if we can? :shrug: ), with an approach that you're comfortable with wrt to size/complexity, that seems worthwhile IMO.

...granted, I don't really need caching on class getters at this point, as they should be cheap anyway, I just don't like the behavioral differences that IMO will confuse users (i.e. see my discussion of "...why does affectedToPathList not have my class getters? is this a bug in valtio? in my code?", and took me ~days to figure out why it was not working)

...I anticipate that you're going to suggest "just use buildProxyFunction to have a newProxy that auto flattenPrototypes only for you"...which, sure, is fine for me I guess (albeit I'll have to tell the team "always use our proxy fn"), but I still think "consistent getter behavior" / "consistent caching behavior" is something Valtio should do out-of-the-box.

stephenh commented 1 year ago

@dai-shi happy to put this on hold; fwiw I assumed it would be a quick win and meant I wouldnt have to explain object-vs-class getters in my docs PR.

But I'll go ahead and leave this as-is, focus on landing the docs PR with the behavior on main today, and then maybe we can revisit this.

dai-shi commented 1 year ago

It's the design decision not to add features in vanilla. It should follow JS behavior as much as possible. So, while I'm fine with the idea of reducing code, strongly against adding things.

I'm also not confident if this is a good default behavior. There can be edge cases that we don't want such behavior. So, it should be opt-in. Granted, it might not be ideal in developer experience. But our bigger focus in this lib is minimal implementation, which should be good for long-term stability and feasible maintainability.

This should fall in to the same pattern as proxyMap/Set.

What would be acceptable is something like the following.

import { withClass } from 'valtio/utils'
const state = proxy(withClass(new FooClass()))
// or
import { proxyWithClass } from 'valtio/utils'
const state = proxyWithClass(new FooClass())

Closing. Please open a new PR or a new discussion.

stephenh commented 1 year ago

I am worried couching this as a feature triggered a bias; imo it is actually a bug fix, b/c:

it fixes the affectedToPathList bug with class getters (verified via a test I pushed to my fork's branch)
it removes any "this is sometimes the snapshot" confusion in class getters (dramatically simplifying valtio's computed properties story, to the extent I would assert that computed properties could be touted as "fully supported"/built-in, vs. the current "we don't provide them but you can try getters only if you're an advanced user"
it aligns object getters & class getters to behave the same
sure it caches as a bonus/new feature, but object getters already do that (granted, it seems by accident more so than on purpose? not sure)

There can be edge cases that we don't want such behavior

Maybe? Seems speculative, unless you do have something concrete in mind (which, if so, object getters would suffer from today).

const state = proxy(withClass(new FooClass()))

Cool, yeah I tried this, but turns out Object.defineProperty-ing getters onto the proxy state technically succeeds, but doesn't actually help, b/c when the new snapshot is Object.create(Counter)-d, we're back to having a prototype-associated property, and snap[key] = value goes straight to it.

So the behavior basically has to live in createSnapshot, because we need to Object.defineProperty directly against the newly created snapshot.

(An alternative would be to drop the original Counter prototype all together, but that also would require changes to createSnapshot, and seems like a regression, probably)

const state = proxyWithClass(new FooClass())

Yeah, that should work with a custom createSnapshot.

I'll work on a proxyWithClass.

dai-shi commented 1 year ago

it aligns object getters & class getters to behave the same

DX-wise, it might be better. But, I think it's how JS works. class getters and object getters are different. I don't think we should do something with prototype implicitly. That's why I don't consider it a bug fix.

My perspective is rather aggressive. If it causes too much trouble, I'd drop class support entirely.

withClass

You probably have a better understanding than I do. I thought, I'd just copy prototype getters to object getters. Not sure why we need to change createSnapshot.

drop the original Counter prototype all together,

I'm probably okay with it, as long as this is an opt-in feature.

stephenh commented 1 year ago

If it causes too much trouble, I'd drop class support entirely.

:-O :hear_no_evil: :-)

I'd just copy prototype getters to object getters. Not sure why we need to change createSnapshot.

I thought so as well, here is that attempt + why it didn't work in some in-source comments:

https://github.com/pmndrs/valtio/compare/main...stephenh:valtio:convert-class-getters?expand=1

tldr is that copying down getters to the proxy object isn't enough; the snapshot, being a new object, needs the getters copied down onto it as well (hence, afaiu, at least some sort of change in createSnapshot, albeit maybe something more elegant than my attempt).

I have a working proxyWithClass but was going to hold off on opening a draft PR, just to let the docs PR land first + let things settle down a bit:

https://github.com/pmndrs/valtio/compare/main...stephenh:valtio:proxy-with-class?expand=1

dai-shi commented 1 year ago

I thought so as well, here is that attempt + why it didn't work in some in-source comments:

https://github.com/pmndrs/valtio/compare/main...stephenh:valtio:convert-class-getters?expand=1

tldr is that copying down getters to the proxy object isn't enough; the snapshot, being a new object, needs the getters copied down onto it as well (hence, afaiu, at least some sort of change in createSnapshot, albeit maybe something more elegant than my attempt).

Thanks for the clear explanation! Now, I think I get what you mean. Yeah, createSnapshot behaves weirdly. I didn't know that. Thanks tons for your patience.

Let me see if I can fix somehow.

I have a working proxyWithClass but was going to hold off on opening a draft PR, just to let the docs PR land first + let things settle down a bit:

https://github.com/pmndrs/valtio/compare/main...stephenh:valtio:proxy-with-class?expand=1

btw, function proxyFunction is good one. I missed it.

pmndrs / valtio

feat: Cache class getter calls. #653

Summary

Check List