The useDropNode hook has an invalid lifecycle

[sbegaudeau]

Issue

The useDropNode hook is responsible for several pieces of behavior related to dragging and dropping nodes. As a result of this complexity, it has many responsibilities which do not work properly.

• It contains a function named onNodeDragStart used by the DiagramRenderer to know which node is being dragged
• It contains a variable named dropFeedbackStyleProvider used by the various nodes to know how the style of the various nodes should be impacted
• It contains a GraphQL query used with useQuery to retrieve compatibility information

As a result of all those responsibilities, this hook is used at several location in the code such as:

• DiagramRenderer
• ImageNode
• ListNode
• RectangularNode
• EllipseNode

By the way, it seems a bit odd that it is not used in IconLabelNode...

With the multiplication of those responsibilities, the hook has now three ways to store pieces of informations with setDropData, setDraggedNode and setDropNodeCompatibilityData. The lifecycle of those individual pieces of state is invalid and creating bugs.

The multiplication of useState / useContext to store things here and there is creating very subtle issues.

Dragged node

If we were to inline the code of this hook to understand its impact a bit better, we have at least five locations in the code which are each storing what they think is the currently dragged node. So in memory, for a diagram displaying "n" nodes, we will have this information stored in "n+1" location and "n" of those will have the wrong information since only the instance of this hook used by the DiagramRenderer will be properly updated.

Compatibility query

We will also have "n+1" location trying to perform calls to useQuery to retrieve the compatibility data, "n" of those are useless since the query depends only the editing context id and the representation id.

Solution

The hook useDropNode should be separated in smaller hooks with a proper lifecycle. The hook useDropNode should only be used by the DiagramRenderer and it should provide the following API:

export interface UseDropNodeValue {
  onNodeDragStart: NodeDragHandler;
  onNodeDrag: NodeDragHandler;
  onNodeDragStop: (onDragCancelled: (node: Node) => void) => NodeDragHandler;
  dropData: NodeDropData;
  diagramBackgroundStyle: DiagramBackgroundStyle;
}

• It should be the only one performing the retrieval of compatibility data
• It should not have any useState<GQLDropNodeCompatibilityEntry[]>([]), it's not necessary
• It should not have both a DropNodeContext and a useState<Node<NodeData> | null>(null), the draggedNode state should probably be removed since it's not necessary

A second hook, which would not perform any GraphQL query nor store any state named useDropNodeStyle(nodeId: string) should be introduced with the following API:

export interface useDropNodeStyleValue {
  style: React.CSSProperties
}

This hook should only request the relevant data from the DropNodeContext

[sbegaudeau]

It should also be noted that errors from the backend are not considered while retrieving the compatibility data. They should be displayed as usual

[sbegaudeau]

Even better, as suggested by @gcoutable the retrieval of the compatibility data could be done in the DiagramRepresentation which already retrieves similar kind of data