xplato / useUndoable

↪ React hook for undo/redo functionality (with batteries included)
MIT License
168 stars 7 forks source link

triggerUpdate strange behaviour (react-flow v10 related) #12

Closed alor closed 2 years ago

alor commented 2 years ago

I've followed the example to make useUndoable working with react-flow-v10, after some problems with the "pixel-to-pixel" movement issue I've managed to handle it. everything works as expected except the last thing that is making me crazy. it seems that when a node Is deleted the triggerUpdate is not able to record the deletion of the connected edges.

live example: https://codesandbox.io/s/react-flow-v10-undo-sjy7tu?file=/src/App.js

if you delete the "second node" (by selecting it and pressing backspace) the state is updated with only two nodes (correctly) but the list of edges remains 2 (should be 0). you can press the "debug" button and look at the current state after the deletion of the node.

I'm trying to understand the issue, but I need some help. it seems that the triggerUpdate correctly receives the empty list of edges upon applyEdgeChanges is invoked. but for some reason the state is not updated correctly and I still have 2 edges in the list in the present state. maybe is a race condition or an internal state that is not updated...

I'm wondering if the triggerUpdate method can have other side effects in other situations.

alor commented 2 years ago

I'm starting wondering if a better approach to support v10 would be to completely ignore (don't record in the history) the state updates in the onNodesChange and onEdgesChange and record in the history only the onConnect onEdgesDelete onNodesDelete so the "undo" action will be consistent in every case ignoring all the select position dimension events fired for apply*Changes

xplato commented 2 years ago

I'm starting wondering if a better approach to support v10 would be to completely ignore (don't record in the history) the state updates in the onNodesChange and onEdgesChange ...

I think you may be right here. The main problem is the generic nature of the aforementioned functions—they handle almost any event, which we have to manually sift through.

Your original issue, however, is interesting. You are correct in that the 2 edges remain in the state after a deletion. If you undo from that point, they are not returned, which means that there is a delay of some kind. I'll do some more investigating, but my first guess would be that the two changes are being overwritten.

Deleting a node affects both the nodes and edges, which is two separate state updates. This could probably be solved by using the specific event handler functions like your most recent comment explained. Nevertheless, I'll continue looking into this issue as it stands.

alor commented 2 years ago

yes. thank you. I also want to find out the first issue with triggerUpdate and then maybe change the code to handle all the events separately to better have a "undo" behaviour for the users. making undo change the selected node is not ideal. selection is not actually an action you usually undo.

xplato commented 2 years ago

I'm closing this as the discussion has moved to issue #13.