After diving deeper, it seems related to when I toggle skipsDiagramUpdate back and forth. My current flow is that when I load canvas data from an api call, I have the flag set to false, the canvas gets updated, and then I switch the flag back to true. While the flag is true, I am tracking the changes incrementally to the canvas via the onModelChange callback and storing in redux. From what it seems like, the transaction history in undo manager is somehow corrupted.
Here is a gif of the bug:
When you see the loading indicator, that is when a save occurs. The backend does not want the add nodes so I strip them out before the call and inject new ones into the response I get back. I overwrite the canvas with the response canvas by toggling skipsDiagramUpdate to false for a single render. Once canvas is overwritten, I go back to skipsDiagramUpdate equal to true. After that, I select the split node and delete it. I then undo the split node deletion and that’s when you’ll start to see the wacky UI. What’s weird is if I go back and forth enough with undo/redo, the canvas then fixes itself by the end of gif. The redux state is always correct and so is the incremental json in onModelChange, for every undo/redo. But the model for the canvas itself seems to be broken after the faulty undo.
Am I doing something wrong with skipsDiagramUpdate? Not sure why I am experiencing this bug. Let me know if there are any followup questions.
That’s complicated enough that I cannot understand the implications of what you are doing. I would not expect to skip anything but recursive updates when you are updating the GoJS state.
If you update to GoJS v3, we no longer use “__gohashid” on data in the model.
I cannot upgrade to v3 for the time being due to legacy code in the same app that would require too many changes due to several custom aspects, most notably the layout. I’ve rewritten the legacy code so I will be able to upgrade at some point in the near future, but only once the legacy code is retired sometime early next year due to product management timelines.
As for skipsDiagramUpdate, I set it to true because I mainly update nodes/links via the diagram’s model object. Setting the flag to true avoids the redux state updates within onModelChange triggering any kind of further GoJS action since the GoJS canvas is already aware of the changes at that point.
My flow is heavily inspired by the gojs-react-basic project. If you look within App.tsx, you’ll see the flag set to true for model changes in onModelChange. However, you then see the flag set to false when changes happen externally. For the case of the project, it’s via the handleInputChange function in App.tsx. For my case, it’s when the api call returns a potentially modified canvas. So the idea of toggling the flag skipsDiagramUpdate back and forth depending on the source of the change seems like a supported flow.
Please let me know if I am misinterpreting the react example and if there is a better solution to overwrite the canvas with the api response containing potentially new nodes/links. Ideally, I’d like to not lose transaction history.
Yes, that’s the expected flow for gojs-react: state updates triggered by external changes should use skipsDiagramUpdate: false, and all GoJS-triggered state updates (namely those in onModelChange) should use skipsDiagramUpdate: true.
Are you sure your state updates are all being done immutably? Mutating data is the most likely cause for that error message.
Ok sounds good, thanks for the confirmation about the flow.
As for state updates, I am not doing state updates immutably from what I can tell. The error is also specifically thrown after diagram.undoManager.undo() is called. The skipsDiagramUpdate flag at that point is correctly set to true and the incremental JSON is correct when onModelChange is triggered.
It seems like the adding of __gohashid to an immutable object is actually happening internally within GoJS. I suspect that me overwriting the entire node/link arrays from the api response is what’s eventually leading to the internal type error. Not honestly sure.
Are copied JavaScript Objects frozen, i.e. totally read-only? If that is it case, our assigning a “__gohashid” so that we can put it into a Set or Map would fail. Call Object.isFrozen to find out. Object.isFrozen() - JavaScript | MDN
The redux state passed in is frozen. This is true though at all times, even during initial load up where the issue does not exist.
Also, aside from the potential issue of objects being frozen, would that explain why the data itself is not matching across the different sources?
diagram.model.nodes, diagram.nodeDataArray and the nodes array stored in redux all do not match. The redux is the only correct list when inspected at the time of the broken UI. The weird thing is that the redux list gets updated via the undo which ultimately drives the onModelChange callback. The callback does in fact have the correct incremental JSON as well.
Here is the snapshot of the canvas I am referring to:
Why are those “add” nodes in the GoJS model? It doesn’t sound like their states need to be saved in your app state (props), so why bother to put them in the Model.nodeDataArray?
At one point, I had the “add nodes” as adornments on incoming nodes but found that there were several key cases in complicated diagrams where the layout didn’t perform the way I ideally wanted.
I had things like node margins come into play but it felt less robust. You can see in this previous post what it used to look like, Tree layout positioning gets messed up.
Having the add nodes as their own node made the layouts cleaner because the add nodes preemptively took up the space where an eventual node would go and GoJS entirely handled positioning. This also made drop zone adornments look cleaner for many cases with less overlapping on other nearby nodes/links because the add node was spaced out enough. Keyboard navigation/interaction is much more straightforward to implement. The biggest advantage may have been the improvement in code readability.
“add nodes” could technically have state. Not yet, but the add node may eventually have a dynamic enabled/disabled state which would also impact styling. It still wouldn’t need to be persisted in the DB though.
I wasn’t aware of unmodeled data at the time I implemented the canvas in question. Can it handle the key cases I am talking about? How would my tree layout handle the positioning of these parts?
The yellow box in Unmodeled Parts section of the Legends and Title link seems to imply that layouts ignore simple parts.
Yes, just call Diagram.add and Diagram.remove to add whatever Nodes you like without using the Model. If you already have a node template defined that you are using, you can just call copy on it in order to have something to add to the Diagram.
Yes, TreeLayout and the other predefined Layouts that make use of a LayoutNetwork only care about Nodes and Links, not simple Parts. But if you have it already laying out the way that you want, then everything is good.
Really the only thing you need to handle is automatically adding and removing those “add” Nodes and connecting them with Links to the Nodes that you want. I assume you already have such a Link template defined for that…
Ok, I will look into using unmodeled data for the add nodes soon.
In regards to the bug, I actually do not see the issue after I do a deep clone of the nodes and links of the redux state passed into the ReactDiagram component. It does seem like that the transaction history is actually correct but because the object is frozen, it hits the error and doesn’t fully complete the undo, hence the inconsistent data and broken canvas.
I likely don’t need any more help on the issue, I’ll report back later today if I am all set.
