From 7a19f500f520914990d179202aac08ede96ed723 Mon Sep 17 00:00:00 2001 From: Mark Amery Date: Mon, 7 Apr 2025 14:48:58 +0100 Subject: [PATCH 1/5] Add tests of broken behaviour --- test/diff/json.js | 45 +++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 45 insertions(+) diff --git a/test/diff/json.js b/test/diff/json.js index e7aae0be..1478003c 100644 --- a/test/diff/json.js +++ b/test/diff/json.js @@ -183,6 +183,51 @@ 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 = []; + diffJson( + {a: ['q', 'r', 's', {t: []}]}, + {a: ['x', 'y', 'z', {bla: []}]}, + {stringifyReplacer: (k, v) => { + calls.push([k, v]); + return v; + }} + ); + expect(calls).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: []}], + ['t', []] + ]); + }); + it("doesn't throw on Object.create(null)", function() { let diff; expect(function() { From 112e61b9f3d0915f175bb4b824a0e871b1fd5811 Mon Sep 17 00:00:00 2001 From: Mark Amery Date: Mon, 7 Apr 2025 14:52:09 +0100 Subject: [PATCH 2/5] Fix one test --- src/diff/json.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/diff/json.js b/src/diff/json.js index 301d1a94..7c66c30e 100644 --- a/src/diff/json.js +++ b/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); From 644781e31ee094d0ef8af7b2be38bef2b73203de Mon Sep 17 00:00:00 2001 From: Mark Amery Date: Mon, 7 Apr 2025 14:56:41 +0100 Subject: [PATCH 3/5] Correct my test and also make it more convincing --- test/diff/json.js | 34 +++++++++++++++++++++++++++++----- 1 file changed, 29 insertions(+), 5 deletions(-) diff --git a/test/diff/json.js b/test/diff/json.js index 1478003c..332076d5 100644 --- a/test/diff/json.js +++ b/test/diff/json.js @@ -201,16 +201,38 @@ describe('diff/json', function() { }); it("should pass the same 'key' values to the replacer as JSON.stringify would", function() { - const calls = []; + const calls = [], + obj1 = {a: ['q', 'r', 's', {t: []}]}, + obj2 = {a: ['x', 'y', 'z', {bla: []}]}; diffJson( - {a: ['q', 'r', 's', {t: []}]}, - {a: ['x', 'y', 'z', {bla: []}]}, + obj1, + obj2, {stringifyReplacer: (k, v) => { calls.push([k, v]); return v; }} ); - expect(calls).to.deep.equal([ + + // 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'], @@ -224,8 +246,10 @@ describe('diff/json', function() { ['1', 'y'], ['2', 'z'], ['3', {bla: []}], - ['t', []] + ['bla', []] ]); + + expect(calls).to.deep.equal(jsonStringifyCalls); }); it("doesn't throw on Object.create(null)", function() { From 1544bd2887c429ce9f2c9899a529413ef2a3e65b Mon Sep 17 00:00:00 2001 From: Mark Amery Date: Mon, 7 Apr 2025 14:58:12 +0100 Subject: [PATCH 4/5] Fix canonicalize; get new test passing --- src/diff/json.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/diff/json.js b/src/diff/json.js index 7c66c30e..94d29e8e 100644 --- a/src/diff/json.js +++ b/src/diff/json.js @@ -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(); From 264e5c3e8fe1bde67e465d2f386b6ada4eb140bd Mon Sep 17 00:00:00 2001 From: Mark Amery Date: Mon, 7 Apr 2025 15:02:28 +0100 Subject: [PATCH 5/5] Add release notes --- release-notes.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/release-notes.md b/release-notes.md index 7b949fb9..581d6349 100644 --- a/release-notes.md +++ b/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