fix #1530 array clear replace should produce one patch

[BrianHung]

What does this PR do and why?

fix #1530

Steps to validate locally

test case added

[coolsoftwaretyler]

Rebased against master to pull in the structural changes but keep Brian's credit on the commit. I'll apply the diffs that make sense and see if I can get this over the finish line. I think this will end up being a breaking change, which works nicely with what we've got going on in https://github.com/mobxjs/mobx-state-tree/discussions/2160.

[coolsoftwaretyler]

Hey @BrianHung - do you have any opinion on this?

Summary of all failing tests
 FAIL  __tests__/core/array.test.ts (13.144 s)
  ● it should emit remove patches

    expect(received).toEqual(expected) // deep equality

    - Expected  - 2
    + Received  + 3

      Array [
        Object {
    -     "op": "remove",
    -     "path": "/0",
    +     "op": "replace",
    +     "path": "",
    +     "value": Array [],
        },
      ]

      109 |   onPatch(doc, (patch) => patches.push(patch))
      110 |   doc.splice(0)
    > 111 |   expect(patches).toEqual([{ op: "remove", path: "/0" }])
          |                   ^
      112 | })
      113 | test("it should apply a remove patch", () => {
      114 |   const { Factory, ItemFactory } = createTestFactories()

      at Object.<anonymous> (__tests__/core/array.test.ts:111:19)

Is this an expected change?

Test case looks like this:

test("it should emit remove patches", () => {
  const { Factory, ItemFactory } = createTestFactories()
  const doc = Factory.create()
  unprotect(doc)
  doc.push(ItemFactory.create())
  let patches: IJsonPatch[] = []
  onPatch(doc, (patch) => patches.push(patch))
  doc.splice(0)
  expect(patches).toEqual([{ op: "remove", path: "/0" }])
})

Is that just coming from this change in the array type?

[BrianHung]

@coolsoftwaretyler

Ah, I remember why its a draft PR. Correctness is a bit ambiguous here because the clear and replace operation of observableArray emits a splice op:

https://github.com/mobxjs/mobx/blob/1b8ab199df5e73d384cc40ec2d13915a690c14f3/packages/mobx/src/types/observablearray.ts#L422-L429

And because the IArrayWillChange only has splice and not clear or replace, it's hard to directly infer what to do; specifically with clear, its equal to splice(0).

https://github.com/mobxjs/mobx/blob/1b8ab199df5e73d384cc40ec2d13915a690c14f3/packages/mobx/src/types/observablearray.ts#L52

Should we emit [{ op: "remove", path: "/0" }] or [{ op: "replace", path: "", value: [] }]?

If applying either operations results in the same end state, then both are acceptable!

[coolsoftwaretyler]

Hmm, I'm seeing different results with those two patches:

import { applyPatch, getSnapshot, t } from "mobx-state-tree";

const inputSnapshot = ["a", "b"];
const array1 = t.array(t.string).create(inputSnapshot);

const snapshot1 = getSnapshot(array1);
console.log("Initial array:", snapshot1);

// [{ op: "remove", path: "/0" }]
applyPatch(array1, [{ op: "remove", path: "/0" }]);

const snapshot2 = getSnapshot(array1);
console.log('[{ op: "remove", path: "/0" }], array snapshot:', snapshot2);

// Make another array with the same snapshot
const array2 = t.array(t.string).create(inputSnapshot);
const snapshot3 = getSnapshot(array2);
console.log("Same starting array:", snapshot3);

// [{ op: "replace", path: "", value: [] }]
applyPatch(array2, [{ op: "replace", path: "", value: [] }]);
const snapshot4 = getSnapshot(array2);
console.log(
  "[{ op: 'replace', path: '', value: [] }], array2 snapshot:",
  snapshot4
);

Results in an output like:

Initial array: (2) ['a', 'b']
[{ op: "remove", path: "/0" }], array snapshot: ['b']
Same starting array: (2) ['a', 'b']
[{ op: 'replace', path: '', value: [] }], array2 snapshot: []

Here's a CodeSandbox: https://codesandbox.io/p/sandbox/pr-2073-patch-operation-demo-dwfnh8

[BrianHung]

@coolsoftwaretyler

I did assume too that the patches would've been emitted from modifying the same starting snapshot. Something like this

import {
  applyPatch,
  getSnapshot,
  t,
  onPatch,
  unprotect,
} from "mobx-state-tree";

const inputSnapshot = ["a", "b"];

const patches1 = [];
const array1 = t.array(t.string).create(inputSnapshot);
onPatch(array1, (patches) => patches1.push(patches));

const patches2 = [];
const array2 = t.array(t.string).create(inputSnapshot);
onPatch(array2, (patches) => patches2.push(patches));

unprotect(array1);
unprotect(array2);

array1.splice(0);
array2.clear();

console.log("array1", getSnapshot(array1), patches1);
console.log("array2", getSnapshot(array2), patches2);

const array3 = t.array(t.string).create(inputSnapshot);
const array4 = t.array(t.string).create(inputSnapshot);

applyPatch(array3, patches1);
applyPatch(array4, patches2);

console.log("array3", getSnapshot(array3));
console.log("array4", getSnapshot(array4));

Are there use cases where you would want to apply patches to a different object than the one they were emitted from?

Don't hesitate to just close this PR btw. I feel like there are ambiguity about correctness that I can see both sides from.

When you apply the "clear" operation on a different object, do you expect

1. it should just remove all values in the target array, because that's what happened to the source array
2. it should only remove indices which were cleared in the source array; any indices and values above that should be kept

I don't have a good answer on this one. :/

[coolsoftwaretyler]

Ah, I think I see what I got wrong. I assumed we were saying that these two patches are equivalent:

[{ op: "remove", path: "/0" }]
[{ op: "replace", path: "", value: [] }]

Which is what I was testing in my CodeSandbox here, and why I was applying the patch to different objects. I wanted to see if those two patches would produce the same end state on two objects that started from an identical snapshot.

But it's not that those two patches are equivalent. In your new example, the patches look like this:

[{ op: "remove", path: "/1" }]
[{ op: "remove", path: "/0" }]

Which combined, have the same effect as

[{ op: "replace", path: "", value: [] }]

So this works as I expect:

import { applyPatch, getSnapshot, t } from "mobx-state-tree";

const inputSnapshot = ["a", "b"];
const array1 = t.array(t.string).create(inputSnapshot);

const snapshot1 = getSnapshot(array1);
console.log("Initial array:", snapshot1);

// [{ op: "remove", path: "/0" }]
applyPatch(array1, [{ op: "remove", path: "/1" }]);
applyPatch(array1, [{ op: "remove", path: "/0" }]);

const snapshot2 = getSnapshot(array1);
console.log('two patches array snapshot:', snapshot2);

// Make another array with the same snapshot
const array2 = t.array(t.string).create(inputSnapshot);
const snapshot3 = getSnapshot(array2);
console.log("Same starting array:", snapshot3);

// [{ op: "replace", path: "", value: [] }]
applyPatch(array2, [{ op: "replace", path: "", value: [] }]);
const snapshot4 = getSnapshot(array2);
console.log(
  "[{ op: 'replace', path: '', value: [] }], array2 snapshot:",
  snapshot4
);

I misunderstood our original conversation and put together an incorrect test.

I will sleep on it a bit, but I think this still works pretty good to fix the original issue it intended. It's a breaking change, but we have a few other breaking changes on deck. I'll probably clean this up, merge it, and at least test it out in a preview build for folks.

Thanks again for all your help!

[coolsoftwaretyler]

Alright, we're gonna call this one a breaking change that fixes a long standing issue, https://github.com/mobxjs/mobx-state-tree/issues/1530, and perhaps even https://github.com/mobxjs/mobx-state-tree/issues/2006, although I have not verified that fix yet.

I'm going to merge this, and we'll release a preview version of 6.0.0 soon, including some other breaking changes/bug fixes that have been introduced recently.

Thanks for this, @BrianHung!