xplato / useUndoable

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

Deep state/mutation issues [with React Flow] #1

Closed xplato closed 2 years ago

xplato commented 2 years ago

Some libraries (ReactFlow, mainly) expect deeper mutation/state updates to properly inform their internal components of state changes. This hook may be using shallow updates which can cause some strange rendering issues.

I'm looking into this.

xplato commented 2 years ago

Could it potentially be a reference (so to speak) issue where we're returning the same state object, like I alluded to here? https://github.com/Infinium8/useUndoable/blob/cecb97e7066653882693ba2814dab4c07d2b1f75/src/mutate.ts#L40-L47

xplato commented 2 years ago

The new react-flow-example folder is, as expected, an example usage of the hook with React Flow. It works just fine, and I'm not sure where we're getting these mutation issues from.

carlosjedwab commented 2 years ago

@xplato I made a Codesandbox to replicate the error: https://codesandbox.io/s/react-flow-undo-forked-wn2kv?file=/src/App.js

In App.js, there are two sections for declaring the elements:

// Uncomment this section to allow the undo/redo feature
  const [elements, setElements, { undo, canUndo, redo, canRedo }] = useUndoable(
    initialElements
  );

  // Uncomment this section to not allow the undo/redo feature
  //const [elements, setElements] = React.useState(initialElements);
  //const undo = () => {};
  //const canUndo = false;
  //const redo = () => {};
  //const canRedo = false;

If the first one is uncommented and the second one is commented, the undo/redu feature will work, but the layouting error will apear. If the first one is commented and the second is uncommented, then the undo/redo feature is not activated, but the layouting works just fine.

(in the second one, the element is a state, and the undo/redo functions are mocked)

carlosjedwab commented 2 years ago

A valid solution I found to fix the issue is to change the onLayout to:

const onLayout = React.useCallback(
    (direction) => {
      const layoutedElements = getLayoutedElements(elements, direction);
      setElements(layoutedElements);

      // unfortunately we need this little hack
      // of adding and deleting something random in the elements
      // to notify react flow about the change.
      setElements([...elements, {}]);
      setElements(elements.slice(0, elements.length));
    },
    [elements, setElements]
  );
xplato commented 2 years ago

Thank you for this. I see the issue. I think it may be some weird nested issue with the fact that the state value from the internal reducer is an object, but I could be wrong.

I'll be working on this today; I'll implement a fix as soon as possible.

xplato commented 2 years ago

It turns out that I was actually right. The comparison check between the present value and the payload was the cause of the mismatched state update.

https://github.com/Infinium8/useUndoable/blob/cecb97e7066653882693ba2814dab4c07d2b1f75/src/mutate.ts#L40-L47

Commenting out lines 40-47 in mutate.ts fixed this issue (and, as I assume, many others). The check exists to prevent unnecessary state updates when passing the same value; however, as my intuition alluded to, this caused more problems than it solved.

Version 3.2.1 (which is live) fixed this issue.

Thanks for letting me know!