Don't generate patch for a no-change set operation

[eriksunsol]

Extending the example in README.md like this:

var myObj = { firstName:"Joachim", lastName:"Wester", contactDetails: { phoneNumbers: [ { number:"555-123" }] } };
var jsonPatcherProxy = new JSONPatcherProxy( myObj );
var observedObject = jsonPatcherProxy.observe(true);
observedObject.firstName = "Albert";
observedObject.firstName = "Albert";
observedObject.firstName = "Albert";
observedObject.contactDetails.phoneNumbers[0].number = "123";
observedObject.contactDetails.phoneNumbers.push({number:"456"});
var patches = jsonPatcherProxy.generate();

results in the following patch:

[
  {"op":"replace","path":"/firstName","value":"Albert"},
  {"op":"replace","path":"/firstName","value":"Albert"},
  {"op":"replace","path":"/firstName","value":"Albert"},
  {"op":"replace","path":"/contactDetails/phoneNumbers/0/number","value":"123"},
  {"op":"add","path":"/contactDetails/phoneNumbers/1","value":{"number":"456"}}
]

The second and third time firstName is set are effectively no-ops which have already been communicated. I think this is wrong. The proposed change recognizes this and renders the following result instead:

[
  {"op":"replace","path":"/firstName","value":"Albert"},
  {"op":"replace","path":"/contactDetails/phoneNumbers/0/number","value":"123"},
  {"op":"add","path":"/contactDetails/phoneNumbers/1","value":{"number":"456"}}
]

[warpech]

I think this is a good change! However, for this PR to be accepted, there need to be tests for the change

[eriksunsol]

Tests added. I have also worked through no-change operations on array elements and made sure undefined and null are treated properly with respect to their JSON projections in objects and arrays (which is slightly different). I took the liberty to add a test or two which are variants of tests that already existed, but for the special cases e.g. when an undefined property is set to null. This is to make sure testing for no-change operations do not catch cases which should be left alone.

[tomalec]

I think we could copy some tests for undefined behavior from fast-json-patch

[tomalec]

I think this function (and tests) need to be extended cover scenario of adding a null beyond the end of the array:

arr = Array(5);
JSON.stringify(arr); // "[null,null,null,null,null]"
arr[7]; // undefined
arr[7] = null; // null - Even though `typeof oldValue === 'undefined'`, this is a significant change, as
JSON.stringify(arr); // [null,null,null,null,null,null,null,null]"

[eriksunsol]

@tomalec Good catch! I noticed that these cases are were not dealt with properly and had intended to file a separate issue about it. The thing is that adding undefined elements is broken on master as well and therefore i figured it was a bit out of scope for this PR.

Check out failing test on 7cf32fd which is based on current master. Here the undefined element results in a replace patch when it should be an add. Based on this PR, the same test also fails, although differently: 5de895f. Here the undefined element is simply ignored. Note that both fail on elements which are added implicitly without ever being set such as arr[5] and arr[6] in the example which are not present in the patch at all.

The test is based on your example with an addition:

arr = Array(5);
JSON.stringify(arr); // "[null,null,null,null,null]"
arr[7]; // undefined
arr[7] = null;
arr[8] = undefined;
// null - Even though `typeof oldValue === 'undefined'`, these are significant changes, as
JSON.stringify(arr); // [null,null,null,null,null,null,null,null,null]"

[eriksunsol]

See #52.

[tomalec]

We found another issue with this function. Is it called when you pop an array?
Then change from undefined(mapped to null) to not existing element.
I guess it should call deleteTrap but it's worth to add a test for such case as well.

[tomalec]

@warpech suggested to make return Reflect.set(tree, key, newValue); DRYier

[warpech]

I will make the required changes in a new PR.

[warpech]

@warpech suggested to make return Reflect.set(tree, key, newValue); DRYier

@tomalec I have made two alternative solutions:

1. single Reflect.set before the big pile of IFs with many return statements: 43f3439
2. single Reflect.set after the big pile of IFs with single return statement: 71078c3

The code is quite complex, and I am not very happy with the readability of either.

Any suggestions which is better? (I can accept "Current" as an answer, too)

[tomalec]

I have a slight preference over 1. because of readable variable names wasKeyInTreeBeforeReflection, valueBeforeReflection and the fact that whether the callback/following code is executed is controlled using standard return, not enigmatic operation.op

[warpech]

As discussed today between @eriksunsol, @tomalec and me, I have pushed my solution that fixes the test created by @eriksunsol (0257a92).

@tomalec, @eriksunsol could you pls check and give your blessing?

I plan to add more tests from https://github.com/Starcounter-Jack/JSON-Patch/tree/master/test in a separate PR before next release.

[tomalec]

Other than code comment and single test to checked and preferably added, LGTM

[tomalec]

We found another issue with this function. Is it called when you pop an array?
Then change from undefined(mapped to null) to not existing element.
I guess it should call deleteTrap but it's worth to add a test for such case as well.

[warpech]

We found another issue with this function. Is it called when you pop an array?
Then change from undefined(mapped to null) to not existing element.
I guess it should call deleteTrap but it's worth to add a test for such case as well.

Let's continue that in #52