Skip to content
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

Merged
merged 6 commits into from
Aug 6, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
40 changes: 25 additions & 15 deletions src/jsonpatcherproxy.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,9 @@

/*!
* https://github.com/Palindrom/JSONPatcherProxy
* (c) 2017 Starcounter
* (c) 2017 Starcounter
* MIT license
*
*
* Vocabulary used in this file:
* * root - root object that is deeply observed by JSONPatcherProxy
* * tree - any subtree within the root or the root
Expand Down Expand Up @@ -35,7 +35,7 @@ const JSONPatcherProxy = (function() {

/**
* Walk up the parenthood tree to get the path
* @param {JSONPatcherProxy} instance
* @param {JSONPatcherProxy} instance
* @param {Object} tree the object you need to find its path
*/
function getPathToTree(instance, tree) {
Expand Down Expand Up @@ -81,32 +81,42 @@ const JSONPatcherProxy = (function() {
Why do we need to check instance._isProxifyingTreeNow?

We need to make sure we mark revocables as inherited ONLY when we're observing,
because throughout the first proxification, a sub-object is proxified and then assigned to
because throughout the first proxification, a sub-object is proxified and then assigned to
its parent object. This assignment of a pre-proxified object can fool us into thinking
that it's a proxified object moved around, while in fact it's the first assignment ever.
that it's a proxified object moved around, while in fact it's the first assignment ever.

Checking _isProxifyingTreeNow ensures this is not happening in the first proxification,
Checking _isProxifyingTreeNow ensures this is not happening in the first proxification,
but in fact is is a proxified object moved around the tree
*/
if (subtreeMetadata && !instance._isProxifyingTreeNow) {
subtreeMetadata.inherited = true;
}

let warnedAboutNonIntegrerArrayProp = false;
const isTreeAnArray = Array.isArray(tree);
const isNonSerializableArrayProperty = isTreeAnArray && !Number.isInteger(+key.toString());

// 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 (isNonSerializableArrayProperty) {
// This happens in Vue 1-2 (should not happen in Vue 3). See: https://github.com/vuejs/vue/issues/427, https://github.com/vuejs/vue/issues/9259
console.warn(`JSONPatcherProxy noticed a non-integer property ('${key}') was set for an array. This interception will not emit a patch. The value is an object, but it was not proxified, because it would not be addressable in JSON-Pointer`);
warnedAboutNonIntegrerArrayProp = true;
}
else {
instance._parenthoodMap.set(newValue, { parent: tree, key });
newValue = instance._proxifyTreeRecursively(tree, newValue, key);
}
}
// let's start with this operation, and may or may not update it later
const operation = {
op: 'remove',
path: pathToKey
};
const isTreeAnArray = Array.isArray(tree);
if (typeof newValue == 'undefined') {
// applying De Morgan's laws would be a tad faster, but less readable
if (!isTreeAnArray && !tree.hasOwnProperty(key)) {
Expand All @@ -127,10 +137,10 @@ const JSONPatcherProxy = (function() {
}
}
} else {
if (isTreeAnArray && !Number.isInteger(+key.toString())) {
if (isNonSerializableArrayProperty) {
/* array props (as opposed to indices) don't emit any patches, to avoid needless `length` patches */
if(key != 'length') {
console.warn('JSONPatcherProxy noticed a non-integer prop was set for an array. This will not emit a patch');
if(key != 'length' && !warnedAboutNonIntegrerArrayProp) {
tomalec marked this conversation as resolved.
Show resolved Hide resolved
console.warn(`JSONPatcherProxy noticed a non-integer property ('${key}') was set for an array. This interception will not emit a patch`);
}
return Reflect.set(tree, key, newValue);
}
Expand Down Expand Up @@ -161,7 +171,7 @@ const JSONPatcherProxy = (function() {
if (subtreeMetadata) {
if (subtreeMetadata.inherited) {
/*
this is an inherited proxy (an already proxified object that was moved around),
this is an inherited proxy (an already proxified object that was moved around),
we shouldn't revoke it, because even though it was removed from path1, it is still used in path2.
And we know that because we mark moved proxies with `inherited` flag when we move them

Expand All @@ -186,9 +196,9 @@ const JSONPatcherProxy = (function() {
}
}
/**
* Creates an instance of JSONPatcherProxy around your object of interest `root`.
* Creates an instance of JSONPatcherProxy around your object of interest `root`.
* @param {Object|Array} root - the object you want to wrap
* @param {Boolean} [showDetachedWarning = true] - whether to log a warning when a detached sub-object is modified @see {@link https://github.com/Palindrom/JSONPatcherProxy#detached-objects}
* @param {Boolean} [showDetachedWarning = true] - whether to log a warning when a detached sub-object is modified @see {@link https://github.com/Palindrom/JSONPatcherProxy#detached-objects}
* @returns {JSONPatcherProxy}
* @constructor
*/
Expand Down
21 changes: 19 additions & 2 deletions test/spec/proxySpec.js
Original file line number Diff line number Diff line change
Expand Up @@ -467,7 +467,6 @@ describe('proxy', function() {
});

it('should not generate a patch when array props are added or replaced - and log a warning', function() {

const obj = [];
const jsonPatcherProxy = new JSONPatcherProxy(obj);
const observedObj = jsonPatcherProxy.observe(true);
Expand All @@ -476,7 +475,25 @@ describe('proxy', function() {

observedObj.lastName = 'Wester';

expect(console.warn).toHaveBeenCalledWith('JSONPatcherProxy noticed a non-integer prop was set for an array. This will not emit a patch');
expect(console.warn).toHaveBeenCalledWith("JSONPatcherProxy noticed a non-integer property ('lastName') was set for an array. This interception will not emit a patch");
});

it('should not proxify an object that is assigned as an array prop - and log a warning', function() {
const obj = [];
const jsonPatcherProxy = new JSONPatcherProxy(obj);
const observedObj = jsonPatcherProxy.observe(true);

spyOn(console, 'warn');

observedObj.person = {
name: "Albert"
};
observedObj.person.name = "Joachim";

expect(console.warn).toHaveBeenCalledWith("JSONPatcherProxy noticed a non-integer property ('person') was set for an array. This interception will not emit a patch. The value is an object, but it was not proxified, because it would not be addressable in JSON-Pointer");

const patches = jsonPatcherProxy.generate();
expect(patches).toReallyEqual([]);
});

it('should not generate the same patch twice (replace)', function() {
Expand Down