xplato / useUndoable

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

Direct state mutation isn't causing a re-render #2

Closed xplato closed 2 years ago

xplato commented 2 years ago

With the React Flow layout Sandbox, the layout change isn't causing a re-render on undo.

The canUndo bool does indicate that this is possible. Therefore, I think it's just not causing the necessary render.

xplato commented 2 years ago

Upon further analysis, it looks like the initial state is not properly passed into the past array.

Before clicking "horizontal," the elements do not have the xPosition properties (sourcePosition, targetPosition). This is expected because of the initial elements.

After clicking, however, the past array actually does have these values. This indicates that the state mutation isn't properly sending the present state.

xplato commented 2 years ago

If I arbitrarily add a new object to layoutedElements, it is not passed into the past array. This leads me to believe that some portion of the state is being mutated directly.

setElements([...layoutedElements, { hi: 'hey' }]);

The { hi: 'hey' } object is not carried over.

xplato commented 2 years ago

Consider:

const onLayout = useCallback(
    (direction) => {
        console.log({elementsBefore: elements});
        const layoutedElements = getLayoutedElements(elements, direction);
        console.log({elementsAfter: elements});
        setElements([...layoutedElements, {hi: 'hey'}]);
        console.log({elementsAfterAll: elements});
    },
    [elements, setElements]
);

The log values are all identical. I am quite certain (although not positive) that the elements state is being mutated directly somewhere (is dagre able to modify the elements array?).

Therefore, I am quite certain that this is not an issue with useUndoable.

xplato commented 2 years ago

I am going to close this due to inactivity.

alor commented 2 years ago

I had the same issue with getLayoutedElements and the incorrect past history. I fixed it by changing the layouting function to return like this:

let nodes = elements.nodes.map((el) => {
    const nodeWithPosition = dagreGraph.node(el.id);
    el.targetPosition = isHorizontal ? "left" : "top";
    el.sourcePosition = isHorizontal ? "right" : "bottom";

    return {
      ...el,
      position: {
        // We are shifting the dagre node position (anchor=center center) to the top left
        // so it matches the react flow node anchor point (top left).
        x: nodeWithPosition.x - nodeWidth / 2,
        y: nodeWithPosition.y - nodeHeight / 2
      }
    };
  });

so a new element Is created with the new position.

I'm wondering if could be a good idea to cloneDeep (link in lodash) the past states so they cannot be modified by anything iterating over the present state and changing nested attributes. what do you think?