UndoMiddleware saves patches from non-targeted store.

[mikeKlech]

Bug report

• [x] I've checked documentation and searched for existing issues
• [x] I've made sure my project is based on the latest MST version
• [x] Fork this code sandbox or another minimal reproduction.

Sandbox link or minimal reproduction code https://codesandbox.io/s/mobx-state-tree-todolist-forked-kxzoqw Above you can see my minimal reproduction of graphics editor. We have a BoxStore with an UndoMiddleware("history" property) where we save our graphics elements and Focus store(for UI) where we save array of references to these graphics elements. Then we do such steps:

1. Add the new Box to the BoxStore.
2. Focus Box(add reference to this box into Focus store).
3. Remove the Box from the BoxStore(at this step, the reference to the Box invalidates).
4. Undo the previous action(recover the removed Box).

And after last step, not only Box is recovered, but also the reference to this box in the Focus store is recovered too.

From the UndoMiddleware Docs I understand that only changes in targetStore (in out case it is BoxStore) should be tracked. But as I see if the action invokes from the targetStore and this action change the state of secondary store(in our case it is Focus), all the patches (includes the changing of the Focus store) are writing to the UndoMiddleware.

Describe the expected behavior Only patches from targetStore should be written to the UndoMiddleware.

Describe the observed behavior All the patches are written to the UndoMiddleware.

[coolsoftwaretyler]

Hey @mikeKlech - thanks for putting together all the reproduction steps. I think you need to find a way to explicitly set targetStore in the environment of the undo manager.

In the source, it looks like it will look for a targetStore in the environment, or fall back to the root of the tree.

If you look at the examples in the mst-middlewares package, it looks like UndoManager gets called from an external function called setUndoManager, which most of the examples run in the actions block (maybe it would be better to do on a lifecycle hook instead.

In the WithoutUndo - declarative: example, they initialize the manager as an empty object, and then re-initialize it, and pass in { targetStore }, where targetStore comes as an argument to the function, and is called with self from within the actions block.

By using hoisting, you can still expose the external manager through the store itself. I modified your CodeSandbox, and I think I've got it working as expected:

https://codesandbox.io/s/mobx-state-tree-todolist-forked-32yeqt?file=/index.js

Let me know if that solves your issue or not!

[coolsoftwaretyler]

Oh I misread the output of my CodeSandbox, it looks like the focus patch is still there.

I will keep poking around here. The docs for this particular middleware does say that it tracks patches for a given tree. Perhaps it grabs the whole tree no matter what?

[coolsoftwaretyler]

Interesting, I found another example of initializing an UndoManager, where it's set up in volatile, which allows it to have a different environment than the tree itself.

I updated my CodeSandbox at https://codesandbox.io/s/mobx-state-tree-todolist-forked-32yeqt?file=/models/BoxStore.js, and you'll see that it still logs the focus patches.

The README for the middleware does say:

The middlewares serve as example and are supported on a best effort bases. The goal of these middlewares is that if they are critical to your system, you can simply copy paste them and further tailor them towards your specific needs.

I wonder if a better option for this use case is to basically fork the existing UndoManager model, and change the code in https://github.com/mobxjs/mobx-state-tree/blob/master/packages/mst-middlewares/src/undo-manager.ts#L133 to use getParent to set up the recording on the current node and parent.

If you compare this middleware with the similar TimeTraveller middleware, you'll see that it takes a targetPath explicitly and then uses that in its afterCreate hook.

[coolsoftwaretyler]

I'm going to label this a real bug, but I think we should consider this something to be resolved in the middlewares, separate from MobX-State-Tree. We have some plans to give the middleware package more love over in https://github.com/mobxjs/mobx-state-tree/issues/2027. Leaving this open for reference in future efforts on middleware.

[mikeKlech]

Hello @coolsoftwaretyler. Thank you for your answer. Actually I have just realized that you found another bug. I meant a different bug. Look at the index.js file of this fork. In the end of the file you will find the representation of the bug which I found. I left some comments. Pay attantion at STEP 3. Also you can open the console.

Its mean that there is not only problem in the afterCreate hook.

[coolsoftwaretyler]

Thanks @mikeKlech!

I think the main theme here is that the middlewares as provided need some attention, or we need to be clear that they're meant to be examples.

The docs for them suggest that they aren't as stable as MST itself, but lots of folks have come to use and rely on them, so I'm hoping we can find a path forward to fix the bugs and get a better maintenance story.

I'm not sure when all of that will happen, but if you want to try to resolve some of this, a PR is welcome and I'd be happy to help you get started.

[zuhorer]

issue moved to : https://github.com/coolsoftwaretyler/mst-middlewares/issues/4

[coolsoftwaretyler]

Closing this so we can tackle it over in the new repo for mst-middlewares. Thanks, all!