projectstorm / react-diagrams

a super simple, no-nonsense diagramming library written in react that just works
https://projectstorm.cloud/react-diagrams
MIT License
8.45k stars 1.16k forks source link

Issue with LinkModel setTargetPort #971

Open chrys-unito opened 1 year ago

chrys-unito commented 1 year ago

Hi,

Not sure if I've followed all guidelines properly, but we've recently upgraded to a new version of react-diagrams and encountered a new issue based on a change to the setTargetPort method which was made in LinkModel here https://github.com/projectstorm/react-diagrams/commit/8173b70834d3ba15cfee4b15b0515621b210a84c#diff-85d35b77b068838235138041137155ba2b82b9965d0868ac1db8a431fe71f6b3 and which was patched in this version: https://github.com/projectstorm/react-diagrams/commit/e34c73ca00aa18d294267bc0a70527693185771b to account for null ports

We rely on the passed argument port to perform the reportedPosition check below:

this.targetPort = port;
this.fireEvent({ port }, 'targetPortChanged');
if (port?.reportedPosition) { // uses `port`
    this.getPointForPort(port).setPosition(port.getCenter());
}

our issue is that the this.targetPort may have changed after firing the event (and no longer reports its position), but the check is still performed against the port argument and this breaks for us in getPointForPort because the ID of the targetPort has now changed and we can no longer set the position for port.

The reason why targetPort changes in our use case is because when a user links 2 blocks together, we want to insert a node in the middle and effectively the port type changes from a BlockPort to a custom NodePort.

User links: Screen Shot 2022-11-29 at 11 57 33 AM

Node is added: Screen Shot 2022-11-29 at 11 57 45 AM

Any way we could instead rely on this.targetPort in the check ? Effectively becoming:

this.targetPort = port;
this.fireEvent({ port }, 'targetPortChanged');
if (this.targetPort?.reportedPosition) {
    this.getPointForPort(this.targetPort).setPosition(this.targetPort.getCenter());
}

(same for setSourcePort i guess) ??? Before our upgrade to the library, the setTargetPort would only perform the event firing and would not attempt to set the position below:

this.targetPort = port;
this.fireEvent({ port }, 'targetPortChanged');

but now since we're calling this.getPointForPort() we rely on:

getPointForPort(port: PortModel): PointModel {
        if (this.sourcePort !== null && this.sourcePort.getID() === port.getID()) {
            return this.getFirstPoint();
        }
        if (this.targetPort !== null && this.targetPort.getID() === port.getID()) { // targetPort has changed ID ! and we never enter here
            return this.getLastPoint();
        }
        return null;
    }

hence why this change is breaking for us.

[edit*] adding error stack trace - see screenshot where targetPort differs from port: Screen Shot 2022-11-29 at 11 41 13 AM

Or are we perhaps missing something ?

thank you for letting us know