fix: invalidate snap cache when property is removed from object #736
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 5e7403c:
|
src/vanilla.ts
Outdated
| @@ -244,6 +244,7 @@ const buildProxyFunction = ( | |||
| const handler: ProxyHandler<T> = { | |||
| deleteProperty(target: T, prop: string | symbol) { | |||
| const prevValue = Reflect.get(target, prop) | |||
| snapCache.delete(target as Object) | |||
There was a problem hiding this comment.
We never delete the cache elsewhere, so it doesn't feel very nice.
Do you happen to know why deleteProperty doesn't increment the version number?
I think we should increment it if it doesn't.
There was a problem hiding this comment.
Ok, I will try more on that way.
And I found out that although if I increase the target version of deleted property in deleteProperty it is still getting the same version in here.
There was a problem hiding this comment.
Would you be able to add a failing test?
Do you mean that adding a test fails when this symptom has occurred?
There was a problem hiding this comment.
ah, yeah. adding a test to represent #712 issue, which fails in the current main branch, and will be resolved with this PR.
There was a problem hiding this comment.
it is still getting the same version
Is it like reading a stale version from somewhere?
| * [2]. Remove target item should be nested object | ||
| * [3]. The array should be subscribed | ||
| * [4]. The array should pass to snapshot() before the target item removed | ||
| */ |
There was a problem hiding this comment.
@dai-shi I added test and reverted the previous commit.
And wrote the error occurrence conditions.
There was a problem hiding this comment.
Thanks. Can you resolve conflicts and merge the main branch?
There was a problem hiding this comment.
Done! And I will look in to it about another approach that you suggested. 😀
There was a problem hiding this comment.
I made some tries after this, but it didn't worked. (ex- force version update if child object removed)
I think It will be hard to fix without breaking something, as you said.
Do you have any idea about approach to fix it?
There was a problem hiding this comment.
#712 (comment) is my current understanding.
|
Closing as inactive. Let's reopen when there's a progress. |
Related Issues or Discussions
related to
#712
Just like that change [1], I suggest removing snap cache when props are removed from the parent object or array
[1] This change is removing the listeners for removed props from the parent object or array.
FYI: #712 is an issue that has occurred since version 1.8.0 caused by ec44557 or #599
Fixes #712
Summary
Adding remove snap cache logic to
deletePropertyfunctionCheck List
yarn run prettierfor formatting code and docs