xaviergonz / mobx-keystone

A MobX powered state management solution based on data trees with first class support for Typescript, support for snapshots, patches and much more
https://mobx-keystone.js.org
MIT License
554 stars 25 forks source link

UndoMiddleware: undo/redo push of child node into parent array? #265

Closed kingpalethe closed 3 years ago

kingpalethe commented 3 years ago

Thanks for this fantastic library.

I've been experimenting with UndoMiddleware, https://mobx-keystone.js.org/actionMiddlewares/undoMiddleware

Which is working quite well, but I'm struggling with a strategy for how to use it for child nodes.

Here's an example.

I have a PARENT class, "Document":

import { Item} from "./Item";
export class Document extends Model({
  title: prop<string>("Untitled"), 
  items: prop<Item[]>(() => []),
}) {...}

const document = new Document({})
const undoManager = undoMiddleware(document);

and a CHILD class, "Item":

import { Document } from "./Document";
export class Item extends Model({
  text: prop<string>(""), 
 position: prop<number>(0)

}) {
 @computed
  get document() {
    return findParent<Document>(this, (parentNode) => parentNode instanceof Document);
  }
}

I am finding that whenever I make changes to document.title, then I can find that undoManager.canRedo returns true, and I can undo changes to the document with undoManager.undo().

Similarly, if a CHILD item has already been added to the document.items array, then changes to the item, for example, changes to item.text, also generate a "patch" such that I can undo changes to that item.

HOWEVER, I am finding that when adding a NEW child Item to the document.items array, a "patch" is not generated, and I can't undo or redo that change. To be clear: My expected behavior is that, assuming a Document with 0 items, after making a NEW Item, such that document.items.length becomes 1, I could then user undo, and documents.items.length would be reduced to 0.

What would you suggest I do here? I do see onChildAttachedToTarget... https://mobx-keystone.js.org/treeLikeStructure#onchildattachedtotarget---object-fn-child-object----void--void-options--deep-boolean-fireforcurrentchildren-boolean--rundetachdisposers-boolean--void ... and I am wondering if I should somehow try to manually trigger a new patch here?

thanks

xaviergonz commented 3 years ago

it should work... I'll create a unit test later and check

xaviergonz commented 3 years ago

BTW are you adding the new item through a model action?

kingpalethe commented 3 years ago

Ah, thanks for that, yes, the issue was that I was doing document.items.push(item) in a modelAction ... but a modelAction on another class, somewhere else in the tree. I just moved the document.items.push(item) into the Document class, so it looks like this.items.push(item), and now the behavior works as expected. Thanks!