not thought through idea: what if we'd put the skipping = false later - after the action is not applied to the history (addUndoState) - like that it might be possible to add the undo-manager in a more declarative way to the actions too.

.actions(self => {
   return {
       actionName: ({ prop1 }) => undoManger.withoutUndo(() => {
          self.prop1 = prop1;
       }
   }
})

This is amazing @robinfehr can you please tell me more or showing an example of this? Thanks!

@robinfehr I didn't entirely get your last comment? Could you elaborate a bit on it?

@mweststrate was a bit tricky to explain in words, check out the last commit to see what I've meant.
it is now also possible use the withoutUndo within the actions in a declarative way. (see example above)

will add the readme changes in a bit :)

and testing flow's and flow's in combination with the newly added declarative way too. (not sure if it will break)

@chemitaxis I've added the examples to the readme, hope it helps.

@robinfehr Thanks! ;)

quickly tested flow's - it's not working yet but it's not due to the changes within this branch (yet) but due to that, the flow_spawn is somehow not within createActionTrackingMiddleware. will dig deeper.

UPDATE:

Initialize undoManager for flow's within afterCreate

the problem above was due to the adding of the undoManger as a closure like

const undoManger = UndoManger.create({}, { targetStore ...})

because the afterCreate hooks are fired before we add the closure - we miss these spawning actions within the actionTrackingMiddleware. that means that we need to initialize the undoManager before executing the first action (e.g. afterCreate)

you might want to ask - why not just add it as a prop within the outermost model?
the answer is, because when we want to add the undoManager declaratively, we'd have to travel upwards to access the prop so either it's the environment or a closure.

we could add the undoManager within the actions context of the innermost model like

.actions(self => {
        const undoManager = UndoManager.create({}, { targetStore: self })
        getEnv(self).undoManager = undoManager
        return {
          afterCreate() {
              self.bookStore.loadBooks()
          }
        }
    })

but that cannot be the solution though.

maybe a wrapper around the dependency injected props so we'd have the self-context there too could be a solution. like:

export const shop = ShopStore.create(
    {},
    (self) => ({
        undoManager:  UndoManager.create({}, { targetStore: self })
    })
)

Flow's declarative

Because of the current initialization we can only do this - which is not so cool:

        const _loadBooks = flow(function* loadBooks() {
            try {
                const json = yield self.shop.fetch("/books.json")
                updateBooks(json)
                markLoading(false)
            } catch (err) {
                console.error("Failed to load books ", err)
            }
        })

        const loadBooks = () => getEnv(self).undoManager.withoutUndo(() => self._loadBooks())

        return {
            updateBooks,
            _loadBooks, // ugly extra action
            loadBooks
       }

when the initialization problem is solved though we could for example add smth like that:

const loadBooks = getEnv(self).undoManager.withoutUndoFlow(function*() {
            ...
        })

there might be a more elegant solution that is more similar to the normal withoutUndo func.
went for that solution since we need at least the flow_spawn to be recorded before we flag again for a possible stop recording

@mweststrate what do you think - how should we approach that best? I'll certainly need some help here 🔢

UPDATE 2:
within a quick discussion with @mweststrate he mentioned decorators - could be a nice way to avoid these ugly getEnv(self).undoManager.withoutUndo.

@robinfehr so far the PR looks good :)

• I would mention that undo groups will record any change during their execution period).
• I think (with the changes that are just merged to master), that if the loadBooks etc is kicked off from afterAttach instead of afterCreate (or the closure), everything would be recorded correctly. Can you confirm that solves the problem, or do I not understand it correctly?
• can you add unit tests for the new features?

should be ready - included the PR #460 thanks @builden

great, hope merge to master soon:)

@robinfehr thanks!