actionGroup behavior

[nlochschmidt]

This plugin looks very useful 👍. I found the concept of having mutations grouped in the test code but the behavior is unclear to me. I thought the way this works is that the mutation are grouped during undo and redo. However action groups seem to be only grouped during undo. I put together a test case here: https://github.com/nlochschmidt/undo-redo-vuex/blob/test-action-groups/test/test-action-group-undo.js that looks like this:

Initial state:

 addItem -> myAction:addItem -> myAction:addItem -> myAction2:addItem
|------------------------------ done --------------------------------| ↑ | undone |

After first undo (myAction2 group undone):

 addItem -> myAction:addItem -> myAction:addItem -> myAction2:addItem
|--------------------- done --------------------| ↑ |---- undone ----|

After second undo (myAction group undone):

 addItem -> myAction:addItem -> myAction:addItem -> myAction2:addItem
| done | ↑ |--------------------- undone ----------------------------|

After first redo (myAction group restored):

 addItem -> myAction:addItem -> myAction:addItem -> myAction2:addItem
|----------------- done ------------------------| ↑ |---- undone ----|

This is actually where things break, because only the first addItem of the myAction group is restored

After second redo (myAction2 group should be restored):

 addItem -> myAction:addItem -> myAction:addItem -> myAction2:addItem
|----------------- done ---------------------------------------------| ↑ | undone |

So is that expected behavior or a bug? Also what would you suppose should be the behavior if an actionGroup value is used multiple times with other mutations done in between? My guess is, the group should be split and only those mutations up to that point should be undone/restored.

[andrewbeng89]

Hi @nlochschmidt, thanks so much for finding this issue and preparing the test scenario! 👍I must admit that the tests for actionGroups are rather simplistic (as this feature was not really used in the project we were primarily using this for), so really appreciate the time you took to come up with test.

I've traced down the source of the error, and it's due to how the undone stack is tracked and updated during the undo operation. The consequence of this is only observed after the subsequent redo.

This will be resolved very shortly in the next release :)

Once again thanks for the issue and detailed description! (PRs for more comprehensive test coverage will be much appreciated)

Cheers!

[nlochschmidt]

Ok great! For my project I definitely need the action groups, I'll try to provide a PR soon and you can check out my solution and see if you want to merge it in.

[nlochschmidt]

@andrewbeng89 Submitted PR #4 related to this issue. Even if you decide to implement it differently, you might be able to make use of the additional tests.

[nlochschmidt]

@andrewbeng89 just saw your 1.1.2 release 🙏 unfortunately using my advanced test case from #4 it becomes visible that the order of mutations is wrong. That was not visible from the previous test, because all items have been the same 😔. Sorry about that...

[andrewbeng89]

@nlochschmidt thanks for your advanced test case 👍 I've included and test them agains the latest 1.1.3 release. Could you please test this out?

Thanks for the PR as well 😅The solution I used to restore the order of the grouped mutations was to add the reference (popped) mutation at the end of the done and undone lists for redo() and undo() respectively. Alternatively a reduceRight operation could be used to identify the mutations to undo/redo.

[nlochschmidt]

Great work! Thanks for being so responsive. I did a quick test and it seems to work now. It would still be great to have the action groups documented in the README though.

Btw. did you think about what I wrote in the first comment on this issue?

Also what would you suppose should be the behavior if an actionGroup value is used multiple times with other mutations done in between? My guess is, the group should be split and only those mutations up to that point should be undone/restored.

Since we didn't need the undo redo callback functionality which is the main cause for #5 and we need to move fast, I decided to take inspiration from this plugin but completely rewrite and simplify it for our usecase.

Anyway I'll leave it up to you to close this Issue or leave it open until if you think there is more to be done here. Up to you

[andrewbeng89]

Also what would you suppose should be the behavior if an actionGroup value is used multiple times with other mutations done in between? My guess is, the group should be split and only those mutations up to that point should be undone/restored.

I've reimplemented the done/undone stack logic with Array.prototype.reduceRight to handle this scenario in the latest 1.1.5 release 😅