Reference invalidation when undoing array replace operation

[aleksey-shmatov]

Bug report I am trying to re-order items in the array and using replace function for this. There is a safe reference to one of the array items in the model. When action is performed everything works as expected but when I try to undo using UndoRedoMiddleware reference gets invalidated and I get warnings in console regarding access to elements which were removed from the tree. How can I avoid this issue? Reference have to be invalidated if referenced item is removed from array.

• [x] I've checked documentation and searched for existing issues
• [x] I've made sure my project is based on the latest MST version

Sandbox link or minimal reproduction code https://codesandbox.io/s/sharp-tom-d44g8e?file=/src/index.js

Describe the expected behavior Reference should remain valid after undo operation and there should be no warnings

Describe the observed behavior Reference invalidated on undo and there is warning in console regarding access to removed node

[coolsoftwaretyler]

So I don't have a good solution for you (yet!) - but I don't think this is necessarily a problem with the middleware. I can get the same warnings just by using an entirely unadorned applyPatch call.

Here's a CodeSandbox with the same warnings, but no call to fooStore.history.undo(): https://codesandbox.io/s/zealous-dust-zxtowy?file=/src/index.js

I'm gonna keep poking around, but I think the middleware itself might be a red herring here.

[aleksey-shmatov]

Right. Recorded actions from replace call are set of add, remove operations and not exactly reverse of what is replace doing. I wonder if there is a way to delay references invalidation after a bunch of changes has been made. Checked this behaviour in mobx-keystone and it seems there is no such issue there.

[coolsoftwaretyler]

Interesting, the patches coming from sort() don't use replace, but rather just add and remove.

Here's a quick CodeSandbox with an onPatch listener. https://codesandbox.io/s/adoring-austin-qxsjn4?file=/src/index.js

When I searched the issues for replace patch, there are a bunch of prior issues, including https://github.com/mobxjs/mobx-state-tree/issues/1530, which seems particularly relevant.

[coolsoftwaretyler]

Like you said, looks like mobx-keystone has a solve for this. Here's a relevant discussion from that project: https://github.com/xaviergonz/mobx-keystone/issues/449

[coolsoftwaretyler]

Based on the discussion they had in mobx-keystone, I'm going to call this one a likely bug. I don't know exactly how to resolve it, but I think we can maybe take a look at what they did and see if it applies here. Would welcome help from folks.

[zuhorer]

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

[coolsoftwaretyler]

Whoops, we incorrectly tagged as middleware, so we are not going to move this issue. Sorry for the back-n-forth on this!

[coolsoftwaretyler]

Hey folks, we just merged https://github.com/mobxjs/mobx-state-tree/pull/2073, which might fix this issue in 6.0.0 and further. It's technically a breaking change since it's conceivable that downstream users might rely on the existing patch behavior.

I haven't released that build yet, but once we do that (maybe this weekend), we should be able to see if that resolves this scenario.

Thanks for your patience!

[coolsoftwaretyler]

Hey folks, I think this is fixed from #2073. You can verify a fix with version 6.0.0-pre.2 as of today, and eventually the 6.0.0 release will include this fix.

We had to label this as a new major version in case people were relying on the old patch behavior, even in its existing buggy state.

Here's a CodeSandbox I forked from the original reproduction. I updated MobX and MST to latest/pre-release, respectively. No more error!

https://codesandbox.io/p/sandbox/sweet-dawn-ppvhmp

Thanks for bearing with us on this one, and huge thanks to @BrianHung for bringing it home.