UndoManager save the 'recorder.undo()' as one history when action throws error

[songzhenqi]

as the title mentioned. I'm not sure if this is a bug or a deliberate design.

code ：

import { types } from "mobx-state-tree";
import { UndoManager } from "mst-middlewares";

const Model = types
    .model('Model', {
        value: 1,
    })
    .actions(self => {
        function setValue(value: number) {
            self.value = value;
            throw 'some error throw by actions';
        }
        return {
            setValue,
        }
    })

const model = Model.create({})
const undoManager = UndoManager.create({}, { targetStore: model });
try {
    model.setValue(11);
} catch (e) {
    console.log('error =', e);
}

// model.value = 1 is OK, since the `recorder.undo()` executed, 
// but undoManager.canUndo = true is NOT OK, because the `recorder.undo()` is not filter by `filter(call)`, 
// it's become a part of UndoManager's history, 
console.log(`model.value = ${model.value}, undoManager.canUndo = ${undoManager.canUndo}`); 

undoManager.undo();

console.log(`model.value = ${model.value}, undoManager.canUndo = ${undoManager.canUndo}`);

result:

and I dig into UndoManager's source code, I think the problem is call.env = undefined calls too early.

[songzhenqi]

I search the mobx-state-tree issues and find this:mobxjs/mobx-state-tree#2010, maybe the same problem.
@coolsoftwaretyler

[coolsoftwaretyler]

Hey @songzhenqi - thanks for the detailed bug report and diagnosis.

I can't say for sure if this is by design. I inherited this project and didn't write the original code. However, the purpose of spinning out a new project here was so we could make bigger changes without needing to move in lockstep with MST.

So here's my proposal to fix this:

1. Fastest: if you write a PR to change the behavior and include tests that assert that behavior, I will review, merge, and publish a new major version quickly.
2. Slower: I am happy to do that myself, but it may take a while. My priority these days is the primary MST library, but I definitely want to help folks out with the rest of the ecosystem. So if you don't submit a PR, I can get to it when I have some time, but I can't say for sure when.

Let me know what you think. Thanks again!

[songzhenqi]

Both proposal are fine for me .
I'd be happy to do a PR. but it will take some time as I'm not very familiar with the github PR process, maybe a few days.（I'll see it as a learning experience）

[coolsoftwaretyler]

Sounds good! Let me know if you need any help. If you don't end up having time or interest, just let me know and I'll grab it eventually. I'll assign this to you.

[songzhenqi]

@coolsoftwaretyler Thanks for trusting a github newbie! :-）

Before I start the PR, I'd like to discuss two ways to solve the problem. Both I had try and could pass tests

1. replace recorder.undo with skipRecording(() => recorder.undo())
   pro: Don't change the call.env = undefined 's behavior
   con: recorder.undo() invoke applyPatch which is also an action. it will not be filtered then go through the whole undoRedoMiddleware‘s functions, create an new recorder but recording nothing.

Result (add some log to show what happened)

2. When onFinish receive error from actions, execute call.env = undefined after recorder.undo()
   pro: applyPatch will be filtered, no more new recorder .
   con: There are two places clean the env. Makes code a lit bit complex . Maybe it will cause any potential problems.

Code :

    onFinish(call, error) {
                const recorder = call.env!.recorder
                // call.env = undefined  // used be here
                recorder.stop()

                if (error === undefined) {
                    call.env = undefined  // if no errors, clean the env immediately
                    if (groupRecorders.length > 0) {
                        const groupRecorder = groupRecorders[groupRecorders.length - 1]
                        groupRecorder.patches = groupRecorder.patches.concat(recorder.patches)
                        groupRecorder.inversePatches = groupRecorder.inversePatches.concat(
                            recorder.inversePatches
                        )
                    } else {
                        ; (self as any).addUndoState(recorder)
                    }
                } else {
                    recorder.undo()
                    call.env = undefined // if has errors, clean the env after rollback, so applyPatch will be filtered
            }

Let me know what you think. Thanks again.

[coolsoftwaretyler]

Hey @songzhenqi - I think I prefer the second approach. I agree it's more complex, but if you leave in some comments and write tests exercising both code paths, that will make this intention clear and maintainable overall.

Tag me in any PR you make. No rush, I'll give it a review as soon as I can. Thanks!