fix(vanilla): improve snapshot creation ignoring object setters in prototype #656
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 97707c3:
|
|
@stephenh how do you like this? |
|
Size Change: +187 B (0%) Total Size: 55.6 kB
ℹ️ View Unchanged
|
| @@ -108,30 +108,35 @@ const buildProxyFunction = ( | |||
| markToTrack(snap, true) // mark to track | |||
| snapCache.set(target, [version, snap]) | |||
| Reflect.ownKeys(target).forEach((key) => { | |||
There was a problem hiding this comment.
@dai-shi this PR in general looks great!
I copy/pasted in the tests from #653 to see what happens, and what I'm observing is that:
-
For object getters, this PR is actually not necessary, b/c new snapshots have a pure/empty prototype, so the original
snap[key] = valueline finds nothing onObject.prototype, and so does a regular storage ofdouble: 2directly onsnap(no invocation of the proxy's object setter). -
For class getters, the snapshot comes in with
Counter.prototypeand sosnap[key] = valuewould get routed to theCounter.doublesetter method, and so this PR avoids that, and so yeah would definedouble: 2directly onsnap.
However, the reason I say "would" is that Reflect.ownKeys still only sees count as the field it should copy over. If I do a super-hacky:
;[...Reflect.ownKeys(target), 'doubled'].forEach((key) => {
On this line, then this loops sees doubled, and so defineProperty(snap, doubled, 4) runs, and then we end up with the same behavior of object getters, the snap.doubled is the primitive 4 value.
So, that's what I've observed. I'll defer to you on whether you think this PR as-is is still an improvement/simplification (even though afaict it doesn't behaviorally change either object or class getter behavior, which could be fine!), or if you want to nudge it further to pick up doubled (and align the object/class getter behavior).
Thanks!
There was a problem hiding this comment.
This PR itself doesn't change the behavior. My hope is it would make withClass util possible.
There was a problem hiding this comment.
My hope is it would make withClass util possible.
Gotcha. Fwiw I think proxyWithClass is what I would prefer b/c it handles the case of "author has some books" and wanting all authors + books to get the same modified behavior.
With withClass, I think the caller would have to withClass each of the books. I guess withClass could recursively withClass its contents, but then as new books are added (author.books.push(new Book()) I think withClass would be missed.
So, in a good way, I think proxyWithClass is a cleaner/easier way to ensure the upgraded behavior is gotten by the entire proxy tree.
There was a problem hiding this comment.
I'm fine with proxyWithClass. Even so, it can be implemented easily, without unstable_buildProxyFunction, if this PR is merged, or not? I didn't confirm. Oh, I see. You want it recursively. Then, this PR might not help.
There was a problem hiding this comment.
You want it recursively. Then, this PR might not help.
Right, although it would help the proxyWithClass's custom createSnapshot be closer to the real createSnapshot (the main change just being the ownKeys line), so if you wanted to merge it anyway / think this is a good improvement, I think that'd be great.
But also np to close out, but still great to see, as I'll probably use this PR's createSnapshot in the proxyWithClass's version.
There was a problem hiding this comment.
(the main change just being the ownKeys line)
Okay, it's a bit unfortunate for my goal, but yeah, I will merge it anyway.
Maybe there's another way to avoid overriding createSnapshot? don't know yet.
ref: #653 (comment)