should generate nice remove op on array.splice

[Pavek]

Here's the test that fails:

it('should generate nice remove op on array.splice', function() {
          obj = { items: ["a", "b", "c"]};
          var observer = jsonpatch.observe(obj);

          obj.items.splice(1, 1); // <<-- removing second item in-place

          patches = jsonpatch.generate(observer);

          // Expect to have only one 'remove' operation, otherwise patches for big arrays are way too long:
          // essentially it says to replace *every* item and remove last one.
          // In case array items are not trivial, this may cause performance issues too, I guess.
          expect(patches).toEqual([
              { op: 'remove', path: '/items/1' }
          ]);

          obj2 = { items: ["a", "b", "c"]};
          jsonpatch.apply(obj2,patches);
          expect(obj).toEqualInJson(obj2);
      });

Now it fails as following:

duplex generate should generate nice remove on array.splice.
Expected [ { op : 'remove', path : '/items/2' }, { op : 'replace', path : '/items/1', value : 'c' } ] to equal [ { op : 'remove', path : '/items/1' } ].

[ghost]

👍 Same issue here.

[tomalec]

@Pavek Thanks for reporting, and nice test case :) It seems it comes from limitations of Object.observe that we use. I will try to figure something out, maybe try how Array.observe affects performance, and how hard is it to shim.
@warpech have we ever consider Array.observe?

[tomalec]

@Pavek, @winduptoy I have created branch for this fiddling with Array.observe.

@Pavek test case passes with native (Object && Array).observe, I have also added test case with full featured .splice removes and adds many items. Please, check if it works fine for you.
If so, I'll move on with performance and shimming work.

[tomalec]

It seems working without native Array.observe is much more complicated. I created algorithm to generate nicer patches, also form manual generate calls (.compare, .generate, shimmed observer): https://github.com/tomalec/JSON-Patch/tree/issues/65_ArrayObserve_shim
and it works nice with native observers.

The problem comes with shim, as we keep mirror as deep clone so there is no way to check that
for

obj = { items: [{name:"obj1"}, {name:"obj2"}, {name:"obj3"}]};

new obj.items[1] is equal to any of old obj.items[?].

@warpech any ideas?
Maybe we could serve better and optimized patches with native support, and leave it as it was for shimmed? - then, naturally, we would not serve different patches for same scenario for shimmed and native observers; equivalent but not same.

[tomalec]

Related SO issue http://stackoverflow.com/questions/29414710/is-there-any-way-to-preserve-the-order-while-generating-patch-for-json-files

[warpech]

We could keep a shallow clone of arrays to check that. This way you could compare arr.indexOf(obj) with arrShallowClone.indexOf(obj)

[tomalec]

The question is where to keep those clones? as currently we keep only one deepClone of everything as Mirror

[alshakero]

Please see #204 (comment)

I don't think we'll implement a fix for this. Because I think our observation efforts will be mainly focused on JSONPatcherProxy.

Closing but feel free to reopen.

[tomalec]

Why deleteProperty trap is not able to catch it correctly?

const JSONPatcherProxy = require('jsonpatcherproxy');

var myObj = { items: ["a", "b", "c"]};
var jsonPatcherProxy = new JSONPatcherProxy( myObj );
var observedObject = jsonPatcherProxy.observe(true);
myObj.items.splice(1, 1); // <<-- removing second item in-place
var patches = jsonPatcherProxy.generate();

https://runkit.com/tomalec/5b151a476d7288001244bd25

[alshakero]

Why deleteProperty trap is not able to catch it correctly?

Because that's what the JS engine actually does. If you watch the traps in slow-motion (so to say) you see the engine replacing (shifting left) array elements one by one starting from the spliced item until it reaches the end then it removes the last element, which only then triggers the deleteProperty trap.

And this makes sense. You want arr[5] value to be different before and after you do splice(2,1). Right? You want the elements shifted.