Support Comparing Map

[MartyZhou]

Hi @tomalec ,

This pull request is partially for the issue Support for Map and Set . I know you have several concerns for adding map and set support. However, I found it very useful in the project I am working on. Due to our implementation, the index of an array doesn't mean much to the back-end. We need to pass the keys to the server so that we can find the instances by keys and apply the patch to them. At the back end, I am using https://github.com/KevinDockx/JsonPatch which supports dictionary.

It would be great if you have approve this pull request.

Please let me know if you have any suggestions.

Thanks, Marty Zhou

[tomalec]

Hi @MartyZhou , thanks for the contribution! We are pretty concerned about non-JSON types, mainly because we would like to serve the best possible performance for JSON compliant objects. We already have many discussions and problems with relaxing this constraint for undefined. Finally, we agreed to keep the JSON.stringify-like behavior.

I would put a list of requirements, to consider non-standard features (this is my personal list, I'm not sure what @warpech and @alshakero think about it):

1. should not affect performance for standard cases (significantly),
2. should be consistent and easy to explain with other standard and non-standard types (see JSON.stringify),
3. Should keep entire API consistent,
4. should be easy to maintain.

---

1. Your PR adds one if for every compare, we can check the numbers with npm run bench On my machine npm run bench-duplex got slowe, especially for deep objects:

   Benchmark results: ------------------ generate operation Ops/sec: 863941 ±1.69% Ran 49723 times in 0.058 seconds. generate operation and re-apply Ops/sec: 580944 ±1.75% Ran 34319 times in 0.059 seconds. compare operation Ops/sec: 370501 ±1.17% Ran 20733 times in 0.056 seconds. compare operation same but deep objects Ops/sec: 47312522 ±1.21% Ran 2607065 times in 0.055 seconds.

   Benchmark results: ------------------ generate operation Ops/sec: 1190802 ±2.04% Ran 72876 times in 0.061 seconds. generate operation and re-apply Ops/sec: 747788 ±1.59% Ran 44369 times in 0.059 seconds. compare operation Ops/sec: 421570 ±0.61% Ran 22463 times in 0.053 seconds. compare operation same but deep objects Ops/sec: 113416642 ±0.76% Ran 6106732 times in 0.054 seconds.

   ,

2. JSON.stringify(new Map([['key1', { p1: 'value1', p2: 'value2'}]])) === "{}" so we would require new explanation for this and undefined case - do you have an idea for something simple to put into the docs?

3. That's the part that worries me most. Usually jsonpatch.apply(objA, jsonPatch.compare(objA, objB)).isDeepEqual(objB). Now, take should return a replace for a property that exists in both maps case for example:

     var arrayA = [
       ['key1', { p1: 'value1', p2: 'value1'}]
     ];
     var arrayB = [
       ['key1', { p1: 'value1', p2: 'value2'}]
     ];
   
     var mapA = new Map(arrayA);
     var mapB = new Map(arrayB);
   
     var patch = jsonpatch.compare(mapA, mapB);
     expect(patch).toReallyEqual([
       {
         op: 'replace',
         path: '/key1/p2',
         value: 'value2'
       }
     ]);

   So let's try should apply patch which is a result of compare to achieve second object:

   expect(jsonpatch.applyPatch(mapA,  jsonpatch.compare(mapA, mapB))).toReallyEqual(mapB);

   results in:

   Uncaught TypeError: Cannot read property 'p2' of undefined
      at Object.replace (fast-json-patch.js:269)
      at applyOperation (fast-json-patch.js:473)
      at Object.applyPatch (fast-json-patch.js:510)
      at <anonymous>:12:24

// @warpech , @alshakero WDYT about adding should apply patch which is a result of compare to achieve second object as a regular test case for API consistency?

[alshakero]

I agree with @tomalec comments. Great points. What I propose is to create seperate functions called compareMaps and compareSets. My reasons are:

1. It would liberate us from the performance tax when it comes to the standard objects and arrays.
2. It will also preserve our adherence to JSON.stringify standard.
3. Will not affect our API, we thoroughly debated each function name and performance.

[warpech]

The goal of fast-json-patch is to be the fastest JSON-Patch implementation in JavaScript. This comes with the price of reduced compatibility. We cut corners such as assuming strict JSON compatibility in the observed objects.

I am sorry to say that @MartyZhou, but we will not accept PRs that don't align with this strategy.

If you have any suggestion how we can expose some callback for you to handle the serialization of Map and Set outside of fast-json-patch, we can consider that.

Here's one idea: You can implement a toJSON method to your Map and Set. If this method exists, it will be used for serialization by https://github.com/Starcounter-Jack/JSON-Patch/blob/master/src/duplex.ts#L195. The body of this method can be inspired by the discussion in https://stackoverflow.com/questions/31190885/json-stringify-a-set.

[MartyZhou]

Thank you all for the feedback. I will try @warpech's suggestion.