-
-
Notifications
You must be signed in to change notification settings - Fork 252
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
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:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would you be able to add a failing test?
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dai-shi I added test and reverted the previous commit.
And wrote the error occurrence conditions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. Can you resolve conflicts and merge the main branch?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done! And I will look in to it about another approach that you suggested. 😀
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi, are you still around?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#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
deleteProperty
functionCheck List
yarn run prettier
for formatting code and docs