Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Centralize isConnection, isLabel and isRoot Checks #783

Merged
merged 3 commits into from
May 9, 2023

Conversation

philippfromme
Copy link
Contributor

@philippfromme philippfromme commented May 4, 2023

  • export isConnection, isLabel and isRoot checks from ModelUtil and test
  • use these checks
  • checks will be usable by bpmn-js, too

Required by bpmn-io/bpmn-js#1903
Related to bpmn-io/bpmn-js#1901

@philippfromme
Copy link
Contributor Author

@nikku When fixing bpmn-io/bpmn-js#1901 I tried this approach. I'd vote for using actual instanceof checks whenever possible and using is*Like checks when not (eg. CopyPaste). Let me know what you think.

@nikku
Copy link
Member

nikku commented May 4, 2023

What is the benefit of instanceof over proper structural checking (i.e. 'labelTarget' in someObject) that we did in the past? The later works in both instance checks as well as is*Like contexts.

@philippfromme
Copy link
Contributor Author

'labelTarget' in someObject

My first idea was using Object.hasOwn(label, 'labelTarget'). But this still doesn't prevent bugs like bpmn-io/bpmn-js#1902. The respective test doesn't work because the connection is created without waypoints. Checking for instanceof would prevent that.

@philippfromme
Copy link
Contributor Author

As discussed with @nikku I will use Object.hasOwn instead of instanceof as it can lead to issues when dealing with more than one instance of diagram-js.

@nikku
Copy link
Member

nikku commented May 4, 2023

@philippfromme min-dash has you covered .

@philippfromme philippfromme changed the title Export and Use instanceof Checks Centralize isConnection, isLabel and isRoot Checks May 4, 2023
@@ -1,8 +1,13 @@
import {
create,
isModelElement
isElementInstance
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why would we rename this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't have to. One thing I was wondering about is the fact we use this check in Modeling. Doesn't that have the same issue with having more than one diagram-js?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it does. However it was never a problem to date. A safe way is to rather do the check in the elementFactory, hence my prior attempt to move it there.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reverted.

@philippfromme philippfromme marked this pull request as ready for review May 4, 2023 15:02
@bpmn-io-tasks bpmn-io-tasks bot added needs review Review pending and removed in progress Currently worked on labels May 4, 2023
@philippfromme philippfromme requested a review from nikku May 8, 2023 07:27
Copy link
Member

@nikku nikku left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good stuff.

@philippfromme philippfromme merged commit 05d886b into develop May 9, 2023
10 checks passed
@philippfromme philippfromme deleted the instance-of-checks branch May 9, 2023 08:10
@bpmn-io-tasks bpmn-io-tasks bot removed the needs review Review pending label May 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants