fix #4

[songzhenqi]

Ignore patches whose path does not start with the targetStore's path.

I've done my best to make cuts to the unit test code，but it's still very long.

Let me know if you have any ideas.

[coolsoftwaretyler]

I'll take a look today! Thank you!

[coolsoftwaretyler]

Hey @songzhenqi - this is looking good, but your work here has spurred me to take a deeper look at what's going on in MST. I may take a few days to get my head around it, but I expect to either merge this PR or have a separate fix sometime over the weekend, I think.

[coolsoftwaretyler]

Hey @songzhenqi - I found a minor bug here. Matching on the path introduces a subtle possibility for when string matches come back too eagerly. I think this is an edge case, but it makes me wonder if there's an approach we can take that fixes the root problem, which is that UndoManager seems to be observing the entire tree, rather than the subtree it was attached to.

Here's an example of a test that will break expectations with your pull request:

test("#4 - should not saves patches from non-targeted store", () => {
  let UID = 1;
  const Box = types.model("Box", {
    id: types.optional(types.identifier, ""),
  });

  const BoxStore = types
    .model("BoxStore", {
      boxes: types.array(Box),
    })
    .actions((self) => ({
      addRandomBox() {
        const randomBox = Box.create({ id: `box-${UID++}` });
        self.boxes.push(randomBox);
        return randomBox;
      },
      remove(box: any) {
        destroy(box);
      },
    }));

  const Focus = types
    .model("Focus", {
      focusedBoxes: types.array(types.safeReference(Box)),
    })
    .actions((self) => ({
      afterCreate() {
        setUndoManager(self);
      },
      toggleBoxFocus(box: any) {
        if (self.focusedBoxes.includes(box)) {
          self.focusedBoxes.remove(box);
        } else {
          self.focusedBoxes.push(box);
        }
      },
    }));

  const FocusDupe = types
    .model({ focus: Focus, somethingElse: types.number })
    .actions((self) => ({
      setSomethingElse(n: number) {
        self.somethingElse = n;
      },
    }));

  const RootStore = types.model("RootStore", {
    boxStore: BoxStore,
    focus: FocusDupe,
  });

  const store = RootStore.create({
    boxStore: { boxes: [] },
    focus: { focus: { focusedBoxes: [] }, somethingElse: 1 },
  });

  let _undoManager: any = null;
  const setUndoManager = (targetStore: any) => {
    _undoManager = UndoManager.create({}, { targetStore });
  };

  const box = store.boxStore.addRandomBox();
  store.focus.setSomethingElse(2);
  store.focus.focus.toggleBoxFocus(box);

  // don't record `focusedBoxes.push(box)`
  expect(getSnapshot(_undoManager)).toEqual({
    history: [
      {
        patches: [
          { op: "add", path: "/boxStore/boxes/0", value: { id: "box-1" } },
        ],
        inversePatches: [{ op: "remove", path: "/boxStore/boxes/0" }],
      },
    ],
    undoIdx: 1,
  });

  store.boxStore.remove(box);
  // don't record safeReference's side effect : `focusedBoxes.remove(box)`
  expect(getSnapshot(_undoManager)).toEqual({
    history: [
      {
        patches: [
          { op: "add", path: "/boxStore/boxes/0", value: { id: "box-1" } },
        ],
        inversePatches: [{ op: "remove", path: "/boxStore/boxes/0" }],
      },
      {
        patches: [{ op: "remove", path: "/boxStore/boxes/0" }],
        inversePatches: [
          { op: "add", path: "/boxStore/boxes/0", value: { id: "box-1" } },
        ],
      },
    ],
    undoIdx: 2,
  });
});

Notice the nested trees, there is now: store.focus and store.focus.focus. But we don't protect against the first focus, because the path looks matches /focus

I apologize for what might seem like a nitpick, but this would actually cause a very difficult-to-debug error in my own work. We have some nested models, where we have something like: PlayerWrapper and Player, and the path to a Player instance is actually someStore.player.player

That's a poor design decision in my application code, but I think if we merge these changes, we'll swap one issue (undo manager observing too much of the tree) for another issue that would be trickier to debug (undo manager sometimes observing too much of the tree if paths match).

What do you think about changing where we call recordPatches in the onStart method? It looks like we are attaching the recordPatches to call.tree, which may be the root of this issue.

Let me know what you think. I appreciate the time you've put into this already, would love to collaborate on fixing the root problem here.

[songzhenqi]

It took me a while to see what the problem was.

The problem is that the filtering method based on patch's path is not stable.
Since the path is determined by the variable name and not the data structure itself.

I agree with you that we need to address the root cause of the problem, which is that the recorder can't monitor the whole tree.

[songzhenqi]

Things are getting a little more complicated.

I find that we are missing the necessary unit tests when the targetStore is not the root node.

If we attach the recordPatches to call.context, all patch's path will cut to this new root. and we need to Remove getRoot from each applyPatch

While this would pass all unit tests known to date, I'm not sure if there are other implications.

[coolsoftwaretyler]

Hey @songzhenqi - that makes sense. That's part of why I moved this repo out of the MST project. It has never been well tested, and IMO should be treated more like a set of examples rather than a production ready package.

But since this was bundled with MST, I think a lot of people rely on it and expect it to behave as it has previously.

Fortunately, since it's in a new project, I'm willing to do as many major version upgrades as needed. I think we can go ahead and make that change. If we do so, I'd like to see a lot more tests that exercise both explicit and implicit behavior.

If you put together a PR as such, I am happy to review, merge, and ship as version 7.0.0, so we don't break anyone relying on untested, current behavior.

Let me know what you think!

[songzhenqi]

I was going to start another discussion, Since you mentioned that

It has never been well tested, and IMO should be treated more like a set of examples rather than a production ready package.

I thought I'd just state it here.

I think UndoManager needs some improvements. Here is what I have already done in my own project (just passed current tests,)

1. Fix bug: UndoManager do not roll back from action's error when in group state. (yes ,I've tried to write more unit tests when I was fixing the last bug UndoManager save the 'recorder.undo()' as one history when action throws error #15 ， but failed immediately on this one)。
2. Refactor: volatile property groupRecorders is no more an array. since it's length can't be longer than 1. I'm guessing that this might have started out as a nested group but somehow gave up.
3. Feature: UndoManager will roll back from Reaction’s error if you turn on the switch in the environment variable(like includeHooks)
4. BreakingChange: startGroup no longer requires a callback function to be passed in. this one is very personal opinionated. I just feel current startGroup's signature seem like recordGroup —do three job, startgroup, execute callback, and endgroup.

So, if there's a major release planned next, I think it could include more. (^_^)

I‘d like to participate in it, but I'm going on a trip and may not be available for the next few days.

[coolsoftwaretyler]

Hey @songzhenqi - sounds great. To be clear, I'm not planning any work here myself. The middleware package is a low priority for me, but I wanted to keep it available and maintained at some level since I know people use it.

If you want to contribute those changes (along with a full test suite), I'd be happy to review, merge, and release a major version with that. But I don't have any plans to do that myself.

I'm willing to do major version upgrades in this package as often as necessary to make the middleware maximally useful. That's the benefit of separating it from MST itself. But I will only do that if folks are interested in making changes, and I'll want to have comprehensive test suites for the new versions so we can make changes in the future without breaking too often.