xplato / useUndoable

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

Child components does not render when I use useUndoable in react-flow #17

Closed amintabrizi closed 1 year ago

amintabrizi commented 1 year ago

If I use the default useState in react flow I have no problems and when I change the state from child components everything works great, but if I use useUndoable as the state manager it doesn't work and only the state is updated, Even useEffect doesn't work.

const [elements, setElements, { undo, canUndo, redo, canRedo, reset }] = useUndoable({ nodes: defaultNodes, edges: defaultEdges, });

useEffect(() => { console.log(elements.nodes[0]); }, [elements]);

xplato commented 1 year ago

ReactFlow's v10 updates now require that you take the event-based approach first. How are you attaching event handlers to the ReactFlow component and updating setElements?

xplato commented 1 year ago

Here's a Codesandbox showing that this behavior works just fine, all that matters is how you update the elements: https://codesandbox.io/s/use-undoable-native-demo-5lgrbz?file=/src/App.js

amintabrizi commented 1 year ago

I am using useContext to get value of state in child components. This is a example of what I am doing in parent component:

const [elements, setElements, { undo, canUndo, redo, canRedo, reset }] = useUndoable({ nodes: defaultNodes, edges: defaultEdges, });

useEffect(() => { console.log(elements.nodes[0]); }, [elements.nodes]);

const triggerUpdate = useCallback( (t, v) => { setElements((e) => ({ nodes: t === "nodes" ? v : e.nodes, edges: t === "edges" ? v : e.edges, })); }, [setElements] );

const onNodesChange = useCallback( (changes) => { triggerUpdate("nodes", applyNodeChanges(changes, elements.nodes)); }, [triggerUpdate, elements.nodes] );

const onEdgesChange = useCallback( (changes) => { triggerUpdate("edges", applyEdgeChanges(changes, elements.edges)); }, [triggerUpdate, elements.edges] );

const onConnect = useCallback( (connection) => { triggerUpdate("edges", addEdge(connection, elements.edges)); }, [triggerUpdate, elements.edges] );

const onNodeChangeFontFamily = useCallback( (id, value) => { const newState = elements?.nodes.map((node) => { if (node.id === id) { node.data.style = { ...node.data.style, fontFamily: value, }; } return node; }); setElements((e) => ({ nodes: newState, edges: elements.edges, })); }, [elements.edges, elements?.nodes, setElements] );

amintabrizi commented 1 year ago

I want to change font family of node in a child component and get value of new state in another child component. It changes state but does not render child components, so I can not get value of new state

xplato commented 1 year ago

First thing I notice is there may be an error in the onNodeChangeFontFamily function. You're attempting to mutate the state directly with node.data.style = { ... }. Since you're returning the node directly, I'm assuming it's "working" in the sense that it's being applied, but these changes will not be reflected in the state. You may have intended this, I'm not sure.

Can you show me some markup and more importantly how you're calling these functions (in particular the onNodeChangeFontFamily function)?

amintabrizi commented 1 year ago

First thing I notice is there may be an error in the onNodeChangeFontFamily function. You're attempting to mutate the state directly with node.data.style = { ... }. Since you're returning the node directly, I'm assuming it's "working" in the sense that it's being applied, but these changes will not be reflected in the state. You may have intended this, I'm not sure.

Before I use useUndoable It was working well.So I do not think the problem is mutate directly. As I said before, I use context for manage state and its values. So I call onNodeChangeFontFamily like this:

const flow = useContext(flowContext); const handleChangeFont = (fontFamilyValue) => { flow.onNodeChangeFontFamily(elementId, fontFamilyValue); };

and get value of state in another child component like this function: const getNode = flow.getNodeById(id);

`<p style={{ fontFamily: getNode.data.style.fontFamily, }}

some text

`

and this is getNodeById function:

const getNodeById = (id) => { let nodeInfo = elements?.nodes.find((node) => { if (node.id === id) { return node; } return null; }); return nodeInfo; };

xplato commented 1 year ago

useUndoable is more strict than useState; I think it more likely that useUndoable is simply bringing to light some subtle state bugs that would fly with useState before.

Although I am not sure if this will solve your issue, I highly recommend mutating the state with the setElements function. If you don't want to be able to undo the font family change, then set the ignoreAction boolean to true: setElements(newElements, null, true). This has the benefit in that it still updates the primary state, which ensures React can properly understand/render your content with the new state.


What does your React.Context object look like? If you are maintaining a separate copy of the state there, that would cause this. If that's the case, you need to move useUndoable to your context provider and include the state mutation functions in that export.

amintabrizi commented 1 year ago

What does your React.Context object look like? If you are maintaining a separate copy of the state there, that would cause this. If that's the case, you need to move useUndoable to your context provider and include the state mutation functions in that export. <flowContext.Provider value={{ sidebarStatus, handleSidebar, nodes: elements?.nodes, onNodesChange, edges: elements?.edges, onEdgesChange, onConnect, onNodeAdd, onNodeChangeText, onNodeChangeType, tools, onSidebarMouseOver, onSidebarMouseOut, onSidebarClick, getNodeById, onNodeChangeFontSize, onNodeChangeFontStyle, onNodeChangeFontWeight, onNodeChangeTextDecoration, onNodeChangeTextAlignment, onNodeChangeTextColor, onNodeChangeTextHighlight, textColors, setTextColors, textHighlight, setTextHighlight, elements, setElements, undo, redo, canUndo, canRedo, reset, }}

image

amintabrizi commented 1 year ago

useUndoable is more strict than useState; I think it more likely that useUndoable is simply bringing to light some subtle state bugs that would fly with useState before.

Although I am not sure if this will solve your issue, I highly recommend mutating the state with the setElements function. If you don't want to be able to undo the font family change, then set the ignoreAction boolean to true: setElements(newElements, null, true). This has the benefit in that it still updates the primary state, which ensures React can properly understand/render your content with the new state.

I'll try changing setElements like you, hope it works :)

xplato commented 1 year ago

Ahh, okay, I believe I've misunderstood. So you are creating useUndoable in your context provider, correct? Then passing those props down to any subscriber of your FlowContext, yes?

amintabrizi commented 1 year ago

yes Exactly

xplato commented 1 year ago

Okay, I see.

One quick thing: Your onNodeChangeFontFamily is a bit interesting. I'd refactor it keeping the following in mind:

Since you are always updating the state, remove the elements.edges, elements?.nodes from the dependency array and do all your operations in the setElement function itself. Something like:

const onNodeChangeFontFamily = useCallback(
  (id, value) => {
    setElements((elements) => {
        // Do your stuff here

        return {
          nodes: newState,
          edges: elements.edges,
        }
    };
  },
  [setElements]
);

Now that I have all this context, can you more completely describe to me exactly what's going on?

amintabrizi commented 1 year ago

Since you are always updating the state, remove the elements.edges, elements?.nodes from the dependency array and do all your operations in the setElement function itself. Something like:

I removed elements.edges, elements?.nodes but it did not work again. My app is too big and It is complicated and I think it is not useful to find the problem. Isn't it better to make a simple example in Codesandbox?

xplato commented 1 year ago

Yes, an example would be best. As well as an explanation of what the current/expected behavior is.

amintabrizi commented 1 year ago

Sorry that I was able to prepare the link late. I worked on the problem for many hours, but I could not solve the problem properly. As you can see in this link, although the state is changed, The related components are not rendered, and as a result, the information in the child-components cannot be received. https://codesandbox.io/s/fervent-rgb-6m4php?file=/src/App.js

amintabrizi commented 1 year ago

The next problem is, why is the undo button activated after the page is fully loaded, while no changes have happened?

xplato commented 1 year ago

Thank you for taking the time to set up this sandbox. I will investigate the issue at my earliest convenience and let you know what I discover 👍

amintabrizi commented 1 year ago

did you check the problem?

xplato commented 1 year ago

Hey @amintabrizi! I apologize, I have not yet had time to investigate this further.

I will put it on my list today and get back to you tomorrow. Thank you for pinging me!

amintabrizi commented 1 year ago

Could you resolve the problem?, I am still waiting for you :(

xplato commented 1 year ago

My sincere apologies. I have been quite busy as of late. I am taking a look at your sandbox as we speak.

Could you provide a very elaborate description of what exactly is not working? I see the weird issue of canUndo, but other than that everything appears to be working as expected.

amintabrizi commented 1 year ago

It's ok, I'm sorry that I'm bothering you :) There are two problems 1-As you can see, you can not change the font although the state is changing, In fact the child component does not rendered 2-If you reload the page, you can see 'can undo' is activated, I could not find any answer that why it is activated

xplato commented 1 year ago

I'm sorry that I'm bothering you :)

You are totally fine! I am glad to help.

//This useEffect does not work,I do not why?

It appears to be working fine for me.


I have figured it out! The issue was your onFontChange function, as I explained before. You should not be mutating the node directly, you need to manually pop and apply the changes via the updater function.

There were a number of issues:

  1. You were using the elements variable directly instead of using the e variable given in the callback to setElements
  2. You were mutating the node's data directly with node.data = { ... }; this is a no-no in React.
  3. You were mapping over the old elements, which means that every change will be like one step back in time.

I have refactored your function to remove all direct mutations, and now it works as expected:

const onFontChange = (fontValue) => {
    setElements((e) => {
      // Filter out all the nodes besides id#2
      const filtered = e.nodes.filter((node) => node.id !== "2");

      // Grab the second node
      const secondNode = e.nodes.filter((node) => node.id === "2")[0];

      return {
        // Return a new array compiled of these filtered objects
        nodes: [
          ...filtered,
          {
            ...secondNode,
            data: {
              ...secondNode.data,
              fontFamily: fontValue
            }
          }
        ],
        // You must use `e` here, not `elements`
        edges: e.edges
      };
    });
  };

You can view my updated sandbox here: https://codesandbox.io/s/eloquent-noyce-c42in6?file=/src/App.js:2324-2982


As for the issue of canUndo, I am still unsure what that cause is, though I am looking into it now 👍

amintabrizi commented 1 year ago

Thank you The problem is that if we want to go back to the previous state, we have to click on the undo button twice?why?

As for the issue of canUndo, I am still unsure what that cause is, though I am looking into it now

Thanks