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
do not proxify objects that are values of non-patchable array properties #47
Conversation
because such problem happens in Vue 2 and it results in an invalid patch. See: vuejs/vue#427, vuejs/vue#9259
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.
Do we plan to revert this change eventually, once Vue 3 would become mature enough?
as proposed by @tomalec in PR review #47 (review)
All addressed. Pleasee re-review.
I don't think so. This change is useful general and the problem is not specific only to Vue. It just happens that Vue 2 is a very high profile framework that exhibits this problem. |
src/jsonpatcherproxy.js
Outdated
// if the new value is an object, make sure to watch it | ||
if ( | ||
newValue && | ||
typeof newValue == 'object' && | ||
!instance._treeMetadataMap.has(newValue) | ||
) { | ||
instance._parenthoodMap.set(newValue, { parent: tree, key }); | ||
newValue = instance._proxifyTreeRecursively(tree, newValue, key); | ||
if (isTreeAnArray && !Number.isInteger(+key.toString())) { |
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.
cosmetic: Just for my personal readability, we could also create a const isArrayProperty = isTreeAnArray && !Number.isInteger(+key.toString())
not to repeat it here and at l.139. Slightly DRYier vs. one const
variable less. If you find it readable and better this way, let's keep 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.
Done
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.
LGTM
as pointed out in #47 (review)
# Conflicts: # test/spec/proxySpec.js
because the last merge had a conflict incorrectly resolved automatically by p4merge
because such problem happens in Vue 2 and it results in an invalid patch. See: vuejs/vue#427, vuejs/vue#9259