Fix stringifyReplacer (#601)

* Add tests of broken behaviour

* Fix one test

* Correct my test and also make it more convincing

* Fix canonicalize; get new test passing

* Add release notes

Author: ExplodingCabbage

release-notes.md

@@ -10,6 +10,9 @@
 - [#581](https://github.com/kpdecker/jsdiff/pull/581) - **fixed some regex operations used for tokenization in `diffWords` taking O(n^2) time** in pathological cases
 - [#595](https://github.com/kpdecker/jsdiff/pull/595) - **fixed a crash in patch creation functions when handling a single hunk consisting of a very large number (e.g. >130k) of lines**. (This was caused by spreading indefinitely-large arrays to `.push()` using `.apply` or the spread operator and hitting the JS-implementation-specific limit on the maximum number of arguments to a function, as shown at https://stackoverflow.com/a/56809779/1709587; thus the exact threshold to hit the error will depend on the environment in which you were running JsDiff.)
 - [#596](https://github.com/kpdecker/jsdiff/pull/596) - **removed the `merge` function**. Previously JsDiff included an undocumented function called `merge` that was meant to, in some sense, merge patches. It had at least a couple of serious bugs that could lead to it returning unambiguously wrong results, and it was difficult to simply "fix" because it was [unclear precisely what it was meant to do](https://github.com/kpdecker/jsdiff/issues/181#issuecomment-2198319542). For now, the fix is to remove it entirely.
+- [#601](https://github.com/kpdecker/jsdiff/pull/601) - **`diffJson`'s `stringifyReplacer` option behaves more like `JSON.stringify`'s `replacer` argument now.** In particular:
+  * Each key/value pair now gets passed through the replacer once instead of twice
+  * The `key` passed to the replacer when the top-level object is passed in as `value` is now `""` (previously, was `undefined`), and the `key` passed with an array element is the array index as a string, like `"0"` or `"1"` (previously was whatever the key for the entire array was). Both the new behaviours match that of `JSON.stringify`.
 
 ## 7.0.0
 

src/diff/json.js

@@ -10,7 +10,7 @@ jsonDiff.tokenize = lineDiff.tokenize;
 jsonDiff.castInput = function(value, options) {
   const {undefinedReplacement, stringifyReplacer = (k, v) => typeof v === 'undefined' ? undefinedReplacement : v} = options;
 
-  return typeof value === 'string' ? value : JSON.stringify(canonicalize(value, null, null, stringifyReplacer), stringifyReplacer, '  ');
+  return typeof value === 'string' ? value : JSON.stringify(canonicalize(value, null, null, stringifyReplacer), null, '  ');
 };
 jsonDiff.equals = function(left, right, options) {
   return Diff.prototype.equals.call(jsonDiff, left.replace(/,([\r\n])/g, '$1'), right.replace(/,([\r\n])/g, '$1'), options);
@@ -25,7 +25,7 @@ export function canonicalize(obj, stack, replacementStack, replacer, key) {
   replacementStack = replacementStack || [];
 
   if (replacer) {
-    obj = replacer(key, obj);
+    obj = replacer(key === undefined ? '' : key, obj);
   }
 
   let i;
@@ -43,7 +43,7 @@ export function canonicalize(obj, stack, replacementStack, replacer, key) {
     canonicalizedObj = new Array(obj.length);
     replacementStack.push(canonicalizedObj);
     for (i = 0; i < obj.length; i += 1) {
-      canonicalizedObj[i] = canonicalize(obj[i], stack, replacementStack, replacer, key);
+      canonicalizedObj[i] = canonicalize(obj[i], stack, replacementStack, replacer, String(i));
     }
     stack.pop();
     replacementStack.pop();

test/diff/json.js

@@ -183,6 +183,75 @@ describe('diff/json', function() {
       ]);
     });
 
+    it('should only run each value through stringifyReplacer once', function() {
+      expect(
+        diffJson(
+          {foo: '123ab'},
+          {foo: '123xy'},
+          {stringifyReplacer: (k, v) => typeof v === 'string' ? v.slice(0, v.length - 1) : v}
+        )
+      ).to.deep.equal(
+        [
+          { count: 1, value: '{\n', removed: false, added: false },
+          { count: 1, value: '  \"foo\": "123a"\n', added: false, removed: true },
+          { count: 1, value: '  \"foo\": "123x"\n', added: true, removed: false },
+          { count: 1, value: '}', removed: false, added: false }
+        ]
+      );
+    });
+
+    it("should pass the same 'key' values to the replacer as JSON.stringify would", function() {
+      const calls = [],
+            obj1 = {a: ['q', 'r', 's', {t: []}]},
+            obj2 = {a: ['x', 'y', 'z', {bla: []}]};
+      diffJson(
+        obj1,
+        obj2,
+        {stringifyReplacer: (k, v) => {
+          calls.push([k, v]);
+          return v;
+        }}
+      );
+
+      // We run the same objects through JSON.stringify just to make unambiguous when reading this
+      // test that we're checking for the same key/value pairs that JSON.stringify would pass to
+      // the replacer.
+      const jsonStringifyCalls = [];
+      JSON.stringify(
+        obj1,
+        (k, v) => {
+          jsonStringifyCalls.push([k, v]);
+          return v;
+        }
+      );
+      JSON.stringify(
+        obj2,
+        (k, v) => {
+          jsonStringifyCalls.push([k, v]);
+          return v;
+        }
+      );
+
+      expect(jsonStringifyCalls).to.deep.equal([
+        ['', {a: ['q', 'r', 's', {t: []}]}],
+        ['a', ['q', 'r', 's', {t: []}]],
+        ['0', 'q'],
+        ['1', 'r'],
+        ['2', 's'],
+        ['3', {t: []}],
+        ['t', []],
+        ['', {a: ['x', 'y', 'z', {bla: []}]}],
+        ['a', ['x', 'y', 'z', {bla: []}]],
+        ['0', 'x'],
+        ['1', 'y'],
+        ['2', 'z'],
+        ['3', {bla: []}],
+        ['bla', []]
+      ]);
+
+      expect(calls).to.deep.equal(jsonStringifyCalls);
+    });
+
     it("doesn't throw on Object.create(null)", function() {
       let diff;
       expect(function() {