✨ Add diff to compute an op between two docs - #18
Conversation
Produces a json0 op transforming one document into another. Strings are diffed to a single contiguous change (sd/si) so the result composes and transforms cleanly past concurrent edits instead of replacing whole values. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Adds a diff(before, after) helper to the json0 type to generate an operation that transforms one document snapshot into another, with special handling for strings to produce a single contiguous sd/si change (better behavior under concurrent edits).
Changes:
- Added
json.diff()implementation (recursive object diff + contiguous string diff). - Added CoffeeScript tests covering object key changes, value changes, string diffs, nesting, and apply round-trips.
- Extended TypeScript declarations to expose
type.diff(before, after).
Reviewed changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
lib/json0.js |
Introduces json.diff() plus internal helpers for value/object/string diffing. |
test/json0.coffee |
Adds a #diff() test suite verifying expected op shapes and apply round-trips. |
lib/index.d.ts |
Exposes diff(before, after) on the exported type in TS typings. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
diff to compute an op between two docsdiff to compute an op between two docs to exposed API
diff to compute an op between two docs to exposed APIdiff to compute an op between two docs
| // such a value by shape, since a raw value carries no type tag. | ||
| export interface DiffableSubtype { | ||
| diff(before: any, after: any): any; | ||
| isDoc(value: any): boolean; |
There was a problem hiding this comment.
This does make me wonder if we should make some change to track this "properly" as part of the document structure. We already have to hack around this in reedsy-sharedb with some imperfect heuristics. Definitely out of scope of this change, though.
|
|
||
| for (var name in subtypes) { | ||
| var st = subtypes[name] | ||
| if (typeof st.diff === 'function' && st.isDoc && st.isDoc(before) && st.isDoc(after)) { |
There was a problem hiding this comment.
- This is begging for a
isTypeDiffable()helper - Should use a
typeofcheck onisDoctoo - Should we skip the
isDoc()check ifbeforeorafterareundefined? I can't remember off the top of my head howapply()works: basically if I'm allowed to do something like{p: ['does-not-exist', 0], si: 'foo'}, then we should allow anundefinedbefore. If I'm not, then I guess we shouldn't (and now I write it down, I actually assume it's not allowed) — at any rate, please make sure this is recorded in tests
There was a problem hiding this comment.
ok! Confirmed apply throws on si-ing a field that doesn't exist hence isDoc should reject undefined. Adding contract tests on this behavior
| var op = []; | ||
|
|
||
| arrayDiff(before, after, deepEqual).forEach(function(diff) { | ||
| var i; |
There was a problem hiding this comment.
Is there a reason this is declared at the top of this scope instead of inline?
There was a problem hiding this comment.
not really a strong reason just matching ES5 vibes from elsewhere in the file/library but I saw you've left some traces of more modern syntax, I can just inline let i = 0 instead
| op.push({p: path.concat(diff.index), ld: work[diff.index]}); | ||
| work.splice(diff.index, 1); |
There was a problem hiding this comment.
I didn't see a test for the multiple deletion case (I couldn't remember off the top of my head if this code should work), which I'd expect to see.
eg I checked this (green) test locally:
it 'handles list deletions of multiple elements', ->
before = [1, 2, 3, 4]
after = [1, 4]
diff = type.diff before, after
assert.deepEqual after, type.apply before, diff| } else { | ||
| // arrayDiff always frames a reorder as moving its block left, but the same | ||
| // reshape is moving the displaced gap right. Emit whichever needs fewer lms. | ||
| if (diff.howMany <= diff.from - diff.to) { |
There was a problem hiding this comment.
Am I right in thinking howMany is the number of elements being moved from base index from to new base index to? So [1, 2, 3, 4] -> [3, 4, 1, 2] is something like {howMany: 2, from: 0, to: 2}? And then this logic is handling the fact that this needs to map to 2 separate lm ops, and you're doing it in the way that results in fewest by comparing the length of the subarray vs the size of the move?
If I've understood that all correctly, it wasn't immediately obvious from the code comment 😅
There was a problem hiding this comment.
Yea you got it exactly right, it's a small optimization that can be nice in long strings. I'm making the comment a little more explicit
| }; | ||
|
|
||
| /** Compute the op that transforms `before` into `after` */ | ||
| text.diff = function(before, after) { |
There was a problem hiding this comment.
I applaud implementing this yourself, but I do wonder if this is a weirdly non-trivial problem, sufficiently so that we should use a library for this as well? Even Delta outsources diffing, and I feel like if we used the same library, you'd at least get identical string diff behaviour with rich-text, which is probably a good thing (or at least consistent).
There was a problem hiding this comment.
Ok I see. This was actually just the code from older impl. of json0.diff (before subtype delegation) but we might as well not write new potentially wrong/inconsistent code if we can just further delegate to fast-diff :)
| return op | ||
| } | ||
|
|
||
| text.isDoc = function(snapshot) { |
There was a problem hiding this comment.
I wonder if this will read better? At the very least I wouldn't use the word Doc, which has a very specific meaning in sharedb (it's a particular class).
| text.isDoc = function(snapshot) { | |
| text.isOfType = function(snapshot) { |
There was a problem hiding this comment.
I admittedly couldn't make up my mind on how to name this. I also thought of something like owns as in e.g. "text0.owns(snapshot)" but I'll go by your suggestion
|
|
||
|
|
||
| describe '#diff()', -> | ||
| roundTrips = (before, after) -> |
There was a problem hiding this comment.
I feel like it's worth setting up the tests to round trip literally every test. It's cheap to run, and adds sense checks on all the edge cases you've thought to test, which can only be a good thing.
| it 'diffs a renamed key with od + oi', -> | ||
| assert.deepEqual [{p: ['title'], od: 'x'}, {p: ['heading'], oi: 'x'}], type.diff {title: 'x'}, {heading: 'x'} | ||
|
|
||
| it 'diffs nested objects along their path', -> |
There was a problem hiding this comment.
Can we also add a test for type.diff {}, {a: {b: 2}}
| type.diff {x: [9, 1, 2, 3, 4]}, {x: [1, 2, 3, 4, 9]} | ||
|
|
||
| it 'diffs a changed object element with ld + li', -> | ||
| assert.deepEqual [{p: ['x', 1], ld: {v: 1}}, {p: ['x', 1], li: {v: 2}}], |
There was a problem hiding this comment.
I'm not sure I'd expect this behaviour. What's the motivation of this over a deeply addressed oi/od?
There was a problem hiding this comment.
ah this comes from diffArray delegating to arraydiff which just works on a deepEqual basis and emits remove/insert/move diffs with no concept of mutated-in-place elements.
I thought this would be too complicated but actually here's the rule I came up with:
for each remove@i {X} immediately followed by insert@i {Y} of a single element where X and Y are the same type of container (array/object) recurse into diffValue(X, Y) instead of emitting li/ld pair
added contract tests for this behavior (this one in particular now emits a od + oi at the nested path
| return diffObject(path, before, after); | ||
| } | ||
|
|
||
| return [{p: path, od: before, oi: after}]; |
There was a problem hiding this comment.
JSON0 supports more operations.
{p:[path], na:x} - adds x to the number at [path].
{p:[path,offset], si:s} - inserts the string s at offset offset into the string at [path] (uses subtypes internally).
{p:[path,offset], sd:s} - deletes the string s at offset offset from the string at [path] (uses subtypes internally).
I wonder if we should utilise them, but maybe it is safer to just do what you did which is
[{p: path, od: before, oi: after}];
There was a problem hiding this comment.
Interesting point.
About si/sd:
these are already taken care of by the subtyping machinery. We delegate diffing strings to text0 and we get {t: 'text0', o: [...]} back with si/sd inside.
The only caveat is consumers need to register text0 as a subtype or isTypeDiffable would return false and the json0.diff would fall back to coarse od/oi. There's an argument where there is some kind of "specialized fallback" for diffing strings when consumers haven't registered text0 that would handle this case instead of the generic fallback, but I don't think it's worth implementing.
About na, this is a bit trickier.
It is true that no before/after pair emits na ops, and after investigating it turns out it is arguably correct. Essentially the core reason is if you're given a before and an after you can't tell the difference between "set a given number to X" and "increment a given number by X". The only way is to know the intent of the op author, but here we only have the starting and final state of the doc.
So if we emitted na ops as diff output we'd essentially be fabricating an incrementing intent. This could be dangerous in the context of rebasing stale ops (which is the entire reason we need to expose this diff machinery for) because two na ops transform additively via OT and that causes a double increment where there might not have been one.
Think:
- doc is {n: 10}
- Alice submits something like {p: ['n'], od: 10, oi: 12} (sets n to 12)
- Bob concurrently submits {p: ['n'], na: 5} (increments n by 5)
- a new hypothetical
json0.diffbranch emits {p: ['n'], na: 2} for Alice's op - that is transformed over Bob's op, the two
nas are applied consecutively and finally doc is {n: 17}
While the od/oi fallback would result in last-write-wins over na i.e. {n: 12} which is arguably more reasonable as it's what currently happens for concurrent conflicting od/oi vs na
| @@ -1,4 +1,5 @@ | |||
| var deepEqual = require('fast-deep-equal'); | |||
| var arrayDiff = require('arraydiff'); | |||
There was a problem hiding this comment.
Any reason we used that package that hasn't been updated for last 11 years, there is some other one updated 3 years ago https://www.npmjs.com/package/fast-array-diff
There was a problem hiding this comment.
Yea there is actually an explanation of this (and more additional approaches considered for diffing arrays) in the PR description. Basically the one you linked is lacking an equivalent of lm. Which is bad mostly because ld+li kills concurrent edits to the same moved element (and also because any plain reorder results in way larger ops)
| diffsTo {x: [{a: 1}, {v: 1}]}, {x: [{a: 1}, {v: 2}]}, | ||
| [{p: ['x', 1, 'v'], od: 1, oi: 2}] |
There was a problem hiding this comment.
can we add tests like this:
{
x: [
{a: 1},
{v: 1}
]
},
{
x: [
{a: 1},
{b: 1},
{v: 1}
]
}, {
x: [
{a: 1},
{v: 1}
]
},
{
x: [
{a: 1},
{b: 1},
{v: 2}
]
}, {
x: [
{a: 1},
{b: 1},
{v: 1}
]
},
{
x: [
{a: 1},
{v: 1}
]
}, {
x: [
{a: 1},
{b: 1},
{v: 1}
]
},
{
x: [
{a: 1},
{v: 2}
]
}, {
x: [
{a: 1},
{v: 1}
]
},
{
x: [
{a: 1},
{b: 1},
{c: 1},
{v: 1}
]
},
{
x: []
},
{
x: []
}, {
x: {}
},
{
x: null
}, There was a problem hiding this comment.
Ok I've added these.
Two are particularly interesting
- [{a: 1}, {v: 1}] -> [{a: 1}, {b: 1}, {v: 2}] which reads like "edit nested v value to 2 + insert b"
- [{a: 1}, {b: 1}, {v: 1}] -> [{a: 1}, {v: 2}] which reads like "edit nested v value to 2 + remove b"
With the current algorithm, diffs emit a b insert/delete as expected but treats the v change as a naive ld/li. The reason recursion doesn't happen in this case is the detectSubstitutions rule only captures 1 remove -> 1 insert at the same index, while here we have insert/remove of b which shifts v's index; because v's value also changed it's not a "move" and it comes back out of arraydiff bundled into the same multi-element remove/insert as b . So we have {2x insert, 1x remove} and {2x remove, 1x insert} respectively in these two cases => no "substitution" detected => no recursion via diffValue into {v}.
Now, trying to merge a pair insert/remove to recurse into would mean we'd need to guess which pair exactly. In this case it looks obvious which element was edited and which one inserted/removed, but consider:
before: [{id: 7, v: 1}]
after: [{id: 9, w: 0}, {id: 7, v: 2}]
We could read this as "the second element had 'v' changed", it sounds more reasonable to the reader than "the first element had 'id' changed to 9, v deleted and w added"; but the algorithm needs to consistently make this decision and to do that we would need to come up with some kind of similarity metric.
So for now I documented this behavior around the tests. The current behavior is still correct and round trips, only thing the more comprehensive changes to diffArray would net us is slightly smaller operations in such edge cases, at the risk of mis-pairing unrelated elements/recursing for object-diffing into the wrong fields.
There was a problem hiding this comment.
Yeah that's why I added 😂 , because of this we may endup with larger ops sequence than needed. I think fun exercise (not for now, but let's create issue) would be to use something like GumTree algorithm to find node matches and find possible smallest diff.
| fastDiff(before, after).forEach(function(segment) { | ||
| var kind = segment[0]; | ||
| var value = segment[1]; | ||
| if (kind === fastDiff.EQUAL) { | ||
| p += value.length; | ||
| } else if (kind === fastDiff.DELETE) { | ||
| op.push({p: p, d: value}); | ||
| } else { | ||
| op.push({p: p, i: value}); | ||
| p += value.length; | ||
| } | ||
| }); | ||
| return op; |
There was a problem hiding this comment.
should we have the same diff for strings in json0?
There was a problem hiding this comment.
So this is the text0 diff; json0 already reuses it via the subtype path, so json0 does get identical string diffing whenever text0 is registered. Back to my point to the other thread we could implement a string "fallback" for when no text0 has been registered on the consumer end so doing it would be "correct" but arguably redundant? Since this is our fork and we know we will register text0 on the sharedb consumer for the rebase machinery (which is probably gonna stay its one consumer)
There was a problem hiding this comment.
I think we should test some edge cases like:
json0.diff(null, null);
json0.diff(null,{});
json0.diff({},null);
json0.diff(1,'a');which in case of json0 should throw
There was a problem hiding this comment.
Added these, all round trip.
The null cases actually are legit json0 apparently (create() returns null); the last one is a bit more suspicious because clearly scalars aren't json0 docs however that one also round trips. Which leads to a question, since you're suggesting diff should throw. Should json0.create and json0.apply throw too when given that sort of input? json0 has no eager root-type validations anywhere currently.
There was a problem hiding this comment.
I mean 1 is valid json but i always though json0 requires root to be object...
There was a problem hiding this comment.
yea totally reasonable I think that's a sharedb convention, but json0 itself doesn't enforce anything on the root so we'd be adding new invariants across methods for a reason that is arguably unrelated (adding this diff capability).
Another way of looking at this is if somehow diff is given json0.diff(1,'a') input we are not asserting that 1 is a valid json0 doc per se, only that diff handles scalars at root level reasonably well (it emits plain od/oi which vaguely makes sense)
| test '', '' | ||
| test '', 'a' | ||
| test 'a', '' | ||
| test 'a', 'ab' | ||
| test 'ab', 'a' | ||
| test 'ab', 'ac' | ||
| test 'abc', 'ac' | ||
| test 'ac', 'abc' |
There was a problem hiding this comment.
Let's add some more edge cases. Emoji might be tricky as they have 2 bytes.
test '😊a😊', '😊b😊'
test '😊a😊', '😊ac'
test '😊a', '☎️a'
test '😊', '☎️'
test '', '☎️'
test '😊', ''
test 'existing' 'totally changed'
test 'with\nnew line' 'without\tnew tab'
test 'with\nnew line' 'without new line'
test 'with\r\nwindows new line' 'without windows new line'
test 'with\r\nwindows new line' 'with\nlinux new line'
test 'this is something' 'these are something too'
test 'a\u030A' 'å'
test ' a' 'a'
test ' a ' 'a'
test 'a ' 'a'
test 'a' ' a'
test 'a' ' a '
test 'a' 'a '
test 'a' 'a '
test 'a'.repeat(10_000_000) ('b'.repeat(5_000_000) + 'a' + 'b'.repeat(4_999_999)
There was a problem hiding this comment.
Added these, all cases round trip clean except for the last one which opened some pandora box.
So that is a pathological fastdiff case (million chars strings with little to no overlap) and it scales very poorly - essentially o(n^2). So as is that can't just be added to specs as it takes hours, and still minutes if we scale it down to millions/hundreds of thousands chars.
So I added a guard to the text0.diff: if the residual after stripping common prefix/suffix is longer than a threshold, then just treat it as a plain replacement d/i ops. The threshold is kind of a magic number we can tweak, but 4k combined residuals technically keeps fast-diffing time below 50ms.
I think the most defensible argument here is if the two strings have little to nothing in common a complete replace d/i is reasonable anyway rather than being a degraded fallback. And clearly, it is better than failing/timing out and it still round-trips.
Tests now assert that above the threshold we get the two d+i ops (and the test finishes fast) while below the threshold we get the fine fastdiff char-by-char list of ops.
Things to keep in mind:
- crucially, this might not matter too much in
json0because the string fields in this case are more likely headers, metadata etc. not huge prose content. Where this might matter way more isDelta.diffand if we agree on this approach I need to open an issue on this repo to mirror the same guard there. - this perf. issue technically already exists for consumers of
Delta.diff(which usesfastdiffunderneath) and never bit us only because current live callers always diff highly overlapping versions of a document (migrations, track-changes). I think the rebase case will arguably be the first potential offender so it warrants this guard.
There was a problem hiding this comment.
Can we do some testing what could be treshold here? I also do not like the replacement as it measn we basically on ever keystroke save the whole text twice in sd and si
There was a problem hiding this comment.
Hmm can we maybe chunk the work? So basically do the diff on each chunk then transform chunks to match indexes. Like this
- Split into chunks of same length (only the last X chunks may have different length)
- The same length chunk and first chunk with different length just run diff and add to all the indexes (chunk number * chunk length)
- if after chunk is empty string just do operation as delete the whole chunk
- if before chunk is empty just do operation as insert the whole chunk
We can try to normalize the ops then
There was a problem hiding this comment.
I'll clarify scope first because that guard would fire only in way more remote scenarios than every keystroke on the live path. Live keystrokes emit si/sd ops from the editor as usual and are not meant to go through diff.
- this diff will be used by the rebase-correction path for offline stale ops w.r.t server
- there must be a single op, submitted offline (or the result of multiple ops composed into one by ShareDB), that replaces a huge span with unrelated text
- the text must really be almost completely unrelated (little to no common prefix/suffix), something caused by "select all chapter and then paste something entirely different over it" while offline and while a migration was deployed (since that's the only path that would trigger the rebase path)
On the chunking, it's an interesting idea to try and salvage the fine fastdiff in all cases, but I don't think it works here. If you have:
before: "AAAA|BBBB|CCCC" (chunks of 4)
after: "XAAA|ABBB|BCCC|C" (prepend X)
fixed-offset chunks don't move when content shifts (like in this case with an insertion), so every pair ends up different, while the actual diff should just be one char {p: 0, i: 'X'}.
I thought maybe this should be XAAAA|BBBB|CCCC chunking so only the first pair of chunks has a diff and the total diff is the correct one, however the problem is we can't pre-align chunks without paying the O(n*d) cost, because knowing how to align essentially is the fastdiff problem we are solving...
I had Claude simulate fastdiff against different text sizes, here's the quadratic progression:
- residual is the number of chars left to diff against after stripping common prefix/suffix
- dissimilar is the worst case scenario (before and after have completely unrelated char sets)
- shuffled-prose is the realistic-ish scenario where the two strings have some nonzero overlap
| residual (per string) | dissimilar | shuffled-prose |
|---|---|---|
| 500 | 2ms | 2ms |
| 1000 | 5ms | 3.5ms |
| 2000 | 21ms | 14ms |
| 3000 | 47ms | 29ms |
| 4000 | 83ms | 51ms |
| 5000 | 131ms | 79ms |
| 10000 | 724ms | 304ms |
| 20000 | 2.1s | 1.2s |
| 40000 | 8.5s | 4.9s |
the guard in the code atm checks the combined residual of the two strings so it's around the 2k-per-string figure in the table above. Technically 4k combined residuals (the number I picked) takes ~20ms in the absolute worst case for fine-diffing through fastdiff
There was a problem hiding this comment.
this diff will be used by the rebase-correction path for offline stale ops w.r.t server
This you cannot guarantee. This is the observation and assumption it won't happen in the future.
fixed-offset chunks don't move when content shifts (like in this case with an insertion), so every pair ends up...
Yeah, it is not normalised op, the op that chunks would give us would be:
chunk 1: {p:0, i: 'X', d: 'A'}
chunk 2: {p:0, i: 'A', d: 'B'}
chunk 3: {p:0, i: 'B', d: 'C'}
chunk 4: {p:0, i: 'C'}
so final op would be:
[
{p:0, i: 'X', d: 'A'},
{p:4, i: 'A', d: 'B'},
{p:8, i: 'B', d: 'C'},
{p:12, i: 'C'}
]
There was a problem hiding this comment.
From the table i would assume cthe chanuk should be somewhere around 5k chars.
There was a problem hiding this comment.
as discussed, I stripped the max residual guard + algo branching (accepting the potential cost degradation case as we already do in other places where Delta.diff is called) and opened issue with our findings/ideas
What
Adds
type.diff(before, after)— returns a json0 op that transformsbeforeintoafter.Objects: added/removed/changed keys →
oi/od; nested objects recurse along the path.Strings: delegated to the owning subtype via
isOfType, not diffed inline;text0owns strings and now computes its string diff withfast-diff(the same algorithmquill-deltauses for rich-text), so json0 and rich-text produce consistent string ops. A string change becomes a text0 subtype op — composing/transforming cleanly instead of a whole-valueod/oi.Arrays — structural diff via
arraydiff, emittingli/ld/lm. Unchanged elements are retained, so concurrent edits to other elements survive; a relocated element stays an lm (keeps identity). An element changed in place (a single remove+insert at the same index, both the same container kind) recurses intodiffValue— so a changed field becomes a deepoi/odor a nested subtype op, not a whole-elementld+li. Primitives, object↔array kind switches, and batch replaces stay literalld+li. Moves pick the smaller side (a small block to a far end is a couple oflms, not O(n)).Equal values → no-op.
Why
(specifics are TBD but we are gonna need this anyway)
@reedsy/reedsy-sharedb's migration middleware needs to rebase a schema-stale op onto a migrated document shape (diff(applied, migratedApplied)) and$fixupthe submitted op with the change, so a stale edit to a reshaped field still lands and all clients converge. json0 hadapply/transform/invert/composebut nodiff.New dependency:
arraydiffWe need a move-aware structural array diff — one that emits lm for a relocated element, not just insert/delete — otherwise a reordered element becomes ld+li and concurrent edits to it are dropped on rebase.
Other weighted options:
jsondiffpatchalso detects moves, but it's heavyweight (~163KB, +2 transitive deps, needs an objectHash, emits a nested delta we'd have to adapt).fast-array-diff,fast-myers-diff,jsdiff) are insert/delete only.arraydiffis genuinely the one library whose op vocabulary (insert/remove/move) maps almost 1:1 onto li/ld/lm; it has zero sub-deps and is the same diff ShareDB itself has used in production for years. Its one real drawback is age (last release 2016), and it is effectively unmaintained. This sounds acceptable because it's genuinely a small pure function.I also considered:
arraydiffinto the repo, but with no local modifications that only adds maintenance burden over a pinned dependency.Known limitation
Cross-array moves (an element relocating from one array into another) can't be expressed as lm — json0 has no cross-path move — so they diff as ld+li. Concurrent edits to such an element don't carry across. (Parked for discussion.)
Tests
#diff() is a new top-level block, sub-grouped (objects / subtypes / lists / scalars): subtype dispatch (delegates, empty-op drop, one-side-owned fallback), list insert/delete/replace/relocate/smaller-side move, nested recursion, plus a 2000-case round-trip-via-apply fuzzer over random array reshapes. Every
#diff()case now round-trips throughapply()as a further sanity check.