Installing useUndoable with React 18

iamshour commented 2 years ago

I'm trying to install this package with my React 18, which is the latest version of CRA (v.18.1.0), but I'm unable to do so because this package requires peer react@^17.0.2. Error Message below:

Could not resolve dependency:
npm ERR! peer react@"^17.0.2" from use-undoable@3.3.9

Is there any possible way to fix this?

xplato commented 2 years ago

Ah, my bad. I've corrected the react peer dependency to 17 || 18—this should resolve this. If it doesn't, go ahead and re-open this.

Just upgrade use-undoable@3.3.10

iamshour commented 2 years ago

Thank you for taking time to respond back to me, much appreciated! However, I've been trying to implement your package with my setup in order to integrate an undo/redo functionality within, but it's giving me so much headache to be totally honest. My setup consists of a CRA while using React-flow-renderer, which is basically a flow builder application. However, my setup is a bit more advanced than the example you provided when using react-flow-renderer, with functionalities such as removing a node/edge, adding custom nodes/edges, adding and persisting data to nodes, dragging/dropping nodes from a toolbox, etc.. While everything is great so far while working on my project, implementing this undo/redo functionality isn't working at all with so many bugs and errors tbh. Could you provide another example of implementing your solution with react-flow-renderer whilst using the latest versions of both React (18+) and React-flow-renderer (10+) please? And I'll imitate and try to reproduce those lages/bugs/errors in a codesandbox and send you the link shortly if you want as well. Thanks so much for your support, and I'm looking forward to hearing back from you! Best regards, Ali Shour

On Tue, May 17, 2022 at 6:46 PM Tristan @.***> wrote:

Ah, my bad. I've corrected the react peer dependency to 17 || 18—this should resolve this. If it doesn't, go ahead and re-open this.

Just upgrade @.***

— Reply to this email directly, view it on GitHub https://github.com/Infinium8/useUndoable/issues/7#issuecomment-1129032224, or unsubscribe https://github.com/notifications/unsubscribe-auth/AP7ZTTAUYRKP5QRAWLHLSRDVKO5NTANCNFSM5WEJRSMQ . You are receiving this because you authored the thread.Message ID: @.***>

xplato commented 2 years ago

(Seeing as how this new comment is unrelated to the original issue, I'll leave this issue closed—but we can continue to discuss this here)

I can only imagine the bugs, yes. useUndoable is rather primitive and it's really only acting like useState. To further help you, I shall ask for more info.

If you were to replace useUndoable with useState, would you have the same problems? If yes, this is a state design issue. In the README, I spoke of integration with ReactFlow v10. Primarily, I noted that the most important thing is that useUndoable is the only state manager in your entire flow system. If you split it up into two separate useUndoable states, you will encounter bugs.

Apart from the normal undo and redo functions, if replacing useUndoable with useState does fix some bugs, then please provide some relevant code that I can view.

Either way, make sure useUndoable is the exclusive state manager—that'll get you to a starting point.

iamshour commented 2 years ago

Thanks for your time once again, Throughout my setup, I've used both the useNodeState & useEdgeState hooks that ships with react-flow-renderer to persist nodes/edges state const [nodes, setNodes, onNodesChange] = useNodesState(initialNodes) const [edges, setEdges, onEdgesChange] = useEdgesState(initalEdges)

Then I pass the nodes, onNodesChange, edges, onEdgesChange to the returned ReactFlow component.

I'll definitely send you a codeSandbox of my original setup (smaller version) tomorrow because it's a bit late here.

Thank you so much for your help, and looking forward for tomorrow for further details.

Best regards, Ali Shour

On Wed, May 18, 2022, 1:28 AM Tristan @.***> wrote:

(Seeing as how this new comment is unrelated to the original issue, I'll leave this issue closed—but we can continue to discuss this here)

I can only imagine the bugs, yes. useUndoable is rather primitive and it's really only acting like useState. To further help you, I shall ask for more info.

If you were to replace useUndoable with useState, would you have the same problems? If yes, this is a state design issue. In the README, I spoke of integration with ReactFlow v10. Primarily, I noted that the most important thing is that useUndoable is the only state manager in your entire flow system. If you split it up into two separate useUndoable states, you will encounter bugs.

Apart from the normal undo and redo functions, if replacing useUndoable with useState does fix some bugs, then please provide some relevant code that I can overlook.

Either way, make sure useUndoable is the exclusive state manager—that'll get you to a starting point.

— Reply to this email directly, view it on GitHub https://github.com/Infinium8/useUndoable/issues/7#issuecomment-1129384351, or unsubscribe https://github.com/notifications/unsubscribe-auth/AP7ZTTCQIP5PLV5LJ5POXJLVKQMSVANCNFSM5WEJRSMQ . You are receiving this because you authored the thread.Message ID: @.***>

xplato commented 2 years ago

Throughout my setup, I've used both the useNodeState & useEdgeState hooks

Yep, this is where I, too, encountered issues. The unfortunate thing is that integration with a custom state system in ReactFlow v10 is a little more difficult than it used to be.

I'll take a look at your sandbox tomorrow, but for now my suggestion is to try to get rid of all other state hooks. Replicating their methods will usually suffice, but I'll take more indication from your upcoming code.

iamshour commented 2 years ago

Greetings Mr Tristan, The link below corresponds to a very minimal mockup of my current application setup (CRA - React-Flow-Renderer) without any setup with your undo-redo package. https://codesandbox.io/s/tender-dubinsky-s8d6i9?file=/src/App.js I'll shortly send you another sandbox using your package (Undoable) which may highlight the issues I'm currently facing. Thanks, Ali Shour

On Wed, May 18, 2022 at 2:13 AM Tristan @.***> wrote:

Throughout my setup, I've used both the useNodeState & useEdgeState hooks

Yep, this is where I, too, encountered issues. The unfortunate thing is that integration with a custom state system in ReactFlow v10 is a little more difficult than it used to be.

I'll take a look at your sandbox tomorrow, but for now my suggestion is to try to get rid of all other state hooks. Replicating their methods will usually suffice, but I'll take more indication from your upcoming code.

— Reply to this email directly, view it on GitHub https://github.com/Infinium8/useUndoable/issues/7#issuecomment-1129406825, or unsubscribe https://github.com/notifications/unsubscribe-auth/AP7ZTTDSC74D7IP6ZRELOBDVKQRZBANCNFSM5WEJRSMQ . You are receiving this because you authored the thread.Message ID: @.***>

xplato commented 2 years ago

const [nodes, setNodes, onNodesChange] = useNodesState(initialNodes);
const [edges, setEdges, onEdgesChange] = useEdgesState([]);

Are you required to use these? The first thing I would do would be to replace both of those with something like:

const [elements, setElements, { undo, redo }] = useUndoable({
    nodes: [],
    edges: [],
});

This way, it uses one state value but it contains both nodes and edges. I believe you can add the onNodesChange function to the <ReactFlow /> component instead of getting it from their custom hooks.

iamshour commented 2 years ago

Oh I'm sorry I forgot to send you the link of the second codesandbox where I already used your implementation to use the useUndoable package throughout my setup. Link: https://codesandbox.io/s/using-undoable-6dji68 However as you can see, whenever I add many nodes and attempt to delete one of them, it acts weirdly in a buggy way and deletes random nodes and more than one at the same time too.

On Wed, May 18, 2022 at 6:39 PM Tristan @.***> wrote:

const [nodes, setNodes, onNodesChange] = useNodesState(initialNodes);const [edges, setEdges, onEdgesChange] = useEdgesState([]);

Are you required to use these? The first thing I would do would be to replace both of those with something like:

const [elements, setElements, { undo, redo }] = useUndoable({ nodes: [], edges: [],});

This way, it uses one state value but it contains both nodes and edges. I believe you can add the onNodesChange function to the component instead of getting it from their custom hooks.

— Reply to this email directly, view it on GitHub https://github.com/Infinium8/useUndoable/issues/7#issuecomment-1130181380, or unsubscribe https://github.com/notifications/unsubscribe-auth/AP7ZTTESIQ3KMYG76M257KLVKUFMZANCNFSM5WEJRSMQ . You are receiving this because you authored the thread.Message ID: @.***>

iamshour commented 2 years ago

So all in all my sandbox links (minified version of my setup) are: Without using useUndoable: https://codesandbox.io/s/tender-dubinsky-s8d6i9 While using useUndoable: https://codesandbox.io/s/using-undoable-6dji68

On Wed, May 18, 2022 at 7:02 PM Ali Shour @.***> wrote:

Oh I'm sorry I forgot to send you the link of the second codesandbox where I already used your implementation to use the useUndoable package throughout my setup. Link: https://codesandbox.io/s/using-undoable-6dji68 However as you can see, whenever I add many nodes and attempt to delete one of them, it acts weirdly in a buggy way and deletes random nodes and more than one at the same time too.

On Wed, May 18, 2022 at 6:39 PM Tristan @.***> wrote:

const [nodes, setNodes, onNodesChange] = useNodesState(initialNodes);const [edges, setEdges, onEdgesChange] = useEdgesState([]);

Are you required to use these? The first thing I would do would be to replace both of those with something like:

const [elements, setElements, { undo, redo }] = useUndoable({ nodes: [], edges: [],});

This way, it uses one state value but it contains both nodes and edges. I believe you can add the onNodesChange function to the <ReactFlow /> component instead of getting it from their custom hooks.

— Reply to this email directly, view it on GitHub https://github.com/Infinium8/useUndoable/issues/7#issuecomment-1130181380, or unsubscribe https://github.com/notifications/unsubscribe-auth/AP7ZTTESIQ3KMYG76M257KLVKUFMZANCNFSM5WEJRSMQ . You are receiving this because you authored the thread.Message ID: @.***>

xplato commented 2 years ago

Great, thanks for sending that over.

Before I actually get started: Are you required to use ReactFlow v10? If they introduced a feature you need, go ahead and disregard this paragraph. If they didn't, I might suggest sticking with their v9.x.x versions. v10 was introduced with good spirit but it introduces a lot of bugs due to the new state management approach. If you can, you might want to use v9.x.x only.

Anyhow, seeing as how you're asking about v10, I'll continue.

So, one thing is that if you log the elements as they change, you'll notice that when you drag a new node onto the pane, it's creating two separate state events. This is why you'd have to call undo() twice for it to revert.

The reason why it is creating two new state events is because the new node's data is being modified right after creation.

If you reload the sandbox and then drop in a new node, you'd get two logs to the console. They both contain the new item, but their contents are different:

The first log has this info for the new node:

{
  id: "7ee4e6c8-443f-4274-82b8-29f8e6c65656",
  type: "inputNode",
  position: {},
  x: 96.5,
  y: 18.4375,
  sourcePosition: "bottom",
  targetPosition: "top",
  data: {},
  edges: Array(0),
}

The second log has this info for the new item:

{
  id: "7ee4e6c8-443f-4274-82b8-29f8e6c65656",
  type: "inputNode",
  position: {},
  x: 96.5,
  y: 18.4375,
  sourcePosition: "bottom",
  targetPosition: "top",
  data: {},
  width: 166,
  height: 110,
}

(Note that both the data and position fields are included in the log, I just didn't find it necessary to add them here)

As we can see here, the second log contains width and height information. Do you know why this is being added? Is ReactFlow adding it by default?

The deletion works every now and then. I think that if you solve the above problem, this will sort itself out.

I've consistently noticed in this project (useUndoable) that most of the source of bugs are simply unforeseen state mutation issues. In this case, that's precisely what's happening (at least with some of the bugs).

I urge you to add the following to your App.js file (or wherever it is outside of the sandbox):

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

If you actually open the logs up and compare the objects, you'll notice some of the stuff that I did—and therefore you'll understand what went wrong (hopefully).

So, in summary:

I think the source of most of your problems is the fact that objects are being arbitrarily mutated—in particular, upon creation.
I think most of the other problems in your project (namely: deletion of nodes and creation of edges) will be resolved by solving the above.

Finally, to note on moving nodes:

ReactFlow v10 fundamentally changed the way events are passed to their consumers. As a result, we have this annoying "bug" (it's not really a bug) where moving a node 10cm away causes hundreds of state updates. Like I've mentioned before, this is not a flaw with useUndoable or ReactFlow, it's just how it is. To get around this, I suggested using the onNodeDragStop function. The problem with this is that the internal state of ReactFlow and useUndoable will differ.

To be honest, I'm not sure what exactly to do about the node move issue. Short of downgrading to ReactFlow v9, I don't know if there's a simple, concise solution.

I hope some of this helps!

iamshour commented 2 years ago

Hello again, Firstly I would like to thank you once again for your time, effort, and patience to understand my problem and help me sort it out. I'm going to try and solve the issues you mentioned which hopefully (optimistic vibes) will solve the problem from its roots. However, if it didn't work out, I'll find a way to revert backwards to react-flow V.9.x.x which may work out. Thanks so much again, Best wishes & regards!

On Wed, May 18, 2022, 7:47 PM Tristan @.***> wrote:

Great, thanks for sending that over.

Before I actually get started: Are you required to use ReactFlow v10? If they introduced a feature you need, go ahead and disregard this paragraph. If they didn't, I might suggest sticking with their v9.x.x versions. v10 was introduced with good spirit but it introduces a lot of bugs due to the new state management approach. If you can, you might want to use v9.x.x only.

Anyhow, seeing as how you're asking about v10, I'll continue.

So, one thing is that if you log the elements as they change, you'll notice that when you drag a new node onto the pane, it's creating two separate state events. This is why you'd have to call undo() twice for it to revert.

The reason why it is creating two new state events is because the new node's data is being modified right after creation.

If you reload the sandbox and then drop in a new node, you'd get two logs to the console. They both contain the new item, but their contents are different:

The first log has this info for the new node:

{

id: "7ee4e6c8-443f-4274-82b8-29f8e6c65656",

type: "inputNode",

position: {},

x: 96.5,

y: 18.4375,

sourcePosition: "bottom",

targetPosition: "top",

data: {},

edges: Array(0), }

The second log has this info for the new item:

{

id: "7ee4e6c8-443f-4274-82b8-29f8e6c65656",

type: "inputNode",

position: {},

x: 96.5,

y: 18.4375,

sourcePosition: "bottom",

targetPosition: "top",

data: {},

width: 166,

height: 110, }

(Note that both the data and position fields are included in the log, I just didn't find it necessary to add them here)

As we can see here, the second log contains width and height information. Do you know why this is being added? Is ReactFlow adding it by default?

The deletion works every now and then. I think that if you solve the above problem, this will sort itself out.

I've consistently noticed in this project (useUndoable) that most of the source of bugs are simply unforeseen state mutation issues. In this case, that's precisely what's happening (at least with some of the bugs).

I urge you to add the following to your App.js file (or wherever it is outside of the sandbox):

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

If you actually open the logs up and compare the objects, you'll notice some of the stuff that I did—and therefore you'll understand what went wrong (hopefully).

So, in summary:

I think the source of most of your problems is the fact that objects are being arbitrarily mutated—in particular, upon creation.

I think most of the other problems in your project (namely: deletion of nodes and creation of edges) will be resolved by solving the above.

Finally, to note on moving nodes:

ReactFlow v10 fundamentally changed the way events are passed to their consumers. As a result, we have this annoying "bug" (it's not really a bug) where moving a node 10cm away causes hundreds of state updates. Like I've mentioned before, this is not a flaw with useUndoable or ReactFlow, it's just how it is. To get around this, I suggested using the onNodeDragStop function. The problem with this is that the internal state of ReactFlow and useUndoable will differ.

To be honest, I'm not sure what exactly to do about the node move issue. Short of downgrading to ReactFlow v9, I don't know if there's a simple, concise solution.

I hope some of this helps!

— Reply to this email directly, view it on GitHub https://github.com/Infinium8/useUndoable/issues/7#issuecomment-1130250467, or unsubscribe https://github.com/notifications/unsubscribe-auth/AP7ZTTAL43GWQDWUTGXLP5DVKUNJ5ANCNFSM5WEJRSMQ . You are receiving this because you authored the thread.Message ID: @.***>

xplato commented 2 years ago

It's my pleasure, Ali. Feel free to open another issue if you have any other problems. Best of luck :)

iamshour commented 2 years ago

Hello Mr Tristan (Again), The link below is a sandbox of the previous setup but while using the Undoable package. Notice how the undo/redo functions are working fine when we add a new node or edge, but is buggy when we try to delete a node after adding many nodes (deleting coule of nodes from a group of nodes). Thanks again for your assistance, and I'm looking forward to hearing back from you. Best regards, Ali shour.

On Wed, May 18, 2022 at 11:22 AM Ali Shour @.***> wrote:

Greetings Mr Tristan, The link below corresponds to a very minimal mockup of my current application setup (CRA - React-Flow-Renderer) without any setup with your undo-redo package. https://codesandbox.io/s/tender-dubinsky-s8d6i9?file=/src/App.js I'll shortly send you another sandbox using your package (Undoable) which may highlight the issues I'm currently facing. Thanks, Ali Shour

On Wed, May 18, 2022 at 2:13 AM Tristan @.***> wrote:

Throughout my setup, I've used both the useNodeState & useEdgeState hooks

Yep, this is where I, too, encountered issues. The unfortunate thing is that integration with a custom state system in ReactFlow v10 is a little more difficult than it used to be.

I'll take a look at your sandbox tomorrow, but for now my suggestion is to try to get rid of all other state hooks. Replicating their methods will usually suffice, but I'll take more indication from your upcoming code.

— Reply to this email directly, view it on GitHub https://github.com/Infinium8/useUndoable/issues/7#issuecomment-1129406825, or unsubscribe https://github.com/notifications/unsubscribe-auth/AP7ZTTDSC74D7IP6ZRELOBDVKQRZBANCNFSM5WEJRSMQ . You are receiving this because you authored the thread.Message ID: @.***>

xplato commented 2 years ago

Hello again!

I'm taking a look at https://codesandbox.io/s/tender-dubinsky-s8d6i9?file=/src/App.js, but I'm not sure this is the correct one (it's using a different state lib). Could you send me a new sandbox?

xplato / useUndoable

Installing useUndoable with React 18 #7

As we can see here, the second log contains width and height information. Do you know why this is being added? Is ReactFlow adding it by default?

The deletion works every now and then. I think that if you solve the above problem, this will sort itself out.

If you actually open the logs up and compare the objects, you'll notice some of the stuff that I did—and therefore you'll understand what went wrong (hopefully).