refactor(vanilla): simplify handling object descriptor #654
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 399f92d:
|
|
Size Change: -71 B (0%) Total Size: 55.3 kB
ℹ️ View Unchanged
|
|
@stephenh Would you review this please? |
| Object.defineProperty(baseObject, key, desc) | ||
| } else { | ||
| const hasValue = 'value' in desc | ||
| delete desc.value |
There was a problem hiding this comment.
@dai-shi just curious, by why delete desc.value and then reset it?
I had noticed when first diving into this that proxyObject[key] = initialObject[key] invoked the handler's set and did the whole "version"/"notifyUpdate"s process for every key on initialObject.
My curiosity was whether, since we've just created the proxy, there should defacto be no subscribers, maybe we could skip all of that?
I might be missing something else important that the handler.set does, not sure.
There was a problem hiding this comment.
AFAIR, some tests failed.
There was a problem hiding this comment.
Ah sure, it looks like it sets up child proxies (just glancing at the subscribe.test.tsx failure). That makes sense.
Maybe leave an in-source hint for future maintainers/newbies? LIke
// Unset and reset the value to ensure child proxies are wrapped
const hasValue = 'value' in desc
delete desc.value
| @@ -245,9 +245,7 @@ const buildProxyFunction = ( | |||
| value = getUntracked(value) || value | |||
| } | |||
| let nextValue = value | |||
| if (Object.getOwnPropertyDescriptor(target, prop)?.set) { | |||
There was a problem hiding this comment.
Nice! I had came across this code and couldn't really piece together why it was necessary; i.e. why we wanted to avoid the promise/child proxy logic just b/c this was a setter.
I am a little morbidly curious why it was necessary in the 1st place, but that's not super important, and great to see it go away.
There was a problem hiding this comment.
I wanted to avoid handling things if setter exists, because we don't know what the setter does. It can be something really unexpected. But, anyway, this would simplify it. Not really sure what happens...
Sure, totally fine. (And, there are only few who barely follow along.) |
Thought this would be more consistent with #653 (comment).