Refactoring: decouple DI from businessObject

[marstamm]

closes #1472

[marstamm]

I overlooked some issues in the create/Replace behavior of Collapsible Shapes and will fix it

EDIT: done

[nikku]

Great change overall.

I like in particular how straight forward this is and that you introduced a utility to use consistently to retrieve the DI. 💪

[nikku]

As this is a breaking change extensions may rely on, one last question before we merge this: What is our story for backwards compatibility / allowing users to gracefully update?

I could imagine two things:

• Clearly pharsed BREAKING CHANGES in the Changelog (a must!)
• Throwing a meaningful error upon accessing businessObject.di (optional)

[marstamm]

I added the breaking changes to the changelog.

For throwing an error: where would be a good place to start looking into it? Ideally, we would have a Accessor on the business Object. As far as I can tell, they are created in bpmn-moddle and we would have to modify our code everywhere in bpmn-js where new ones are created (import, new elements, replace(?), ...).

[nikku]

We did establish the relationship here and could make that a property that throws.

Object.definedProperty(el, 'di', {
  get: function() {
    throw 'HELLO YOU';
  }
});

My personal opinion is: It does make sense to be verbose about it.

[marstamm]

Thanks for the hint! I decided to create the error in the elementFactory. This way, we also cover newly created elements and not only the imported ones.

Good thing we did it, because I also found 2 places where we still accessed the di incorrectly.

[marstamm]

I hope I selected the correct files to add the tests to 🤞

I kept the previous commit to make it clear what has changed, the commit history should probably be cleaned up anyway before we merge it.

[nikku]

Thanks for your continued follow-ups. The last thing I'm wondering: How could the tests not detect the DI miss-use?

I'll look into your last changes tomorrow and see if we're merge-ready.

[marstamm]

Regarding the tests: we used the (wrong) di to check if it is part of the diPlane. When we accessed it incorrectly, connectionDi was undefined and the test green.

Should we also assert that connectionDi exists in the test cases? Here is the line where connectionDi is used:

bpmn-js/test/spec/features/modeling/AppendShapeSpec.js

Line 285 in b3c7741

expect(subProcessDi.$parent.get('planeElement')).not.to.include(connectionDi);

[nikku]

Let us do that, to verify tests do not accidentally pass again.

We can do so in a assume block.

// given
...

// assume
expect(connectionDi).to.exist;

// when
...

It is a pattern that we use in our code base.

[nikku]

When does this case occur and do we have a test case for it?

[marstamm]

This happens when we replace a Task with an expanded sub-process. The newly created Start event has a di.$parent = null.

We don't test this explicitly, but tests like this check if we can replace Tasks with expanded sub-processes

bpmn-js/test/spec/features/modeling/behavior/SubProcessStartEventBehaviorSpec.js

Lines 29 to 47 in 194b963

	it('should add start event child to subprocess', inject(
	function(elementRegistry, bpmnReplace) {
	// given
	var task = elementRegistry.get('Task_1'),
	expandedSubProcess,
	startEvents;
	// when
	expandedSubProcess = bpmnReplace.replaceElement(task, {
	type: 'bpmn:SubProcess',
	isExpanded: true
	});
	// then
	startEvents = getChildStartEvents(expandedSubProcess);
	expect(startEvents).to.have.length(1);
	}

[nikku]

Thanks for that explaination.

But then, why did it not blow up in the past? I'll investigate and report back.

[nikku]

The root cause here is that DI updating is now broken on replace and makes that quick fix necessary. I'll continue to investigate.

[nikku]

I believe this is the issue. We cannot blindly re-use the existing DI on replace but must continue to create a new one.

[nikku]

Otherwise the following will happen:

• Task (to be replaced) gets removed
• DI (of task and replaced element) is being unlinked
• Replaced element does not have a proper DI reference anymore

[nikku]

Tests for proper DI updating after replace (BpmnReplaceSpec) are missing entirely and need to be added.

[nikku]

I've cleaned up the commit history on this one, added additional test coverage for label wiring (was non-existing in a few places) and found two other small gotchas that I've cleaned up.

At this point, there is one last comment from my side remaining.

Looks great overall. We should proceed and get this out of the door.

[nikku]

Found the last bug ™️.

Please have a look. We're light on DI update test coverage, this is why this one did not blow up on your PR.

[marstamm]

Found the root-cause:

When creating elements in the elementFactory, we supported passing an Object with di Attributes. We now expect the di to be present or absent.
attrs.di is now used for the di moddleElement, so I broke the option to pass an Object.

We should probably fix this, but I'm not sure what's the best way:

1. Overload attrs.di and check if attrs.di is an Object or a Moddle element. Do we have utility functions to check for a Moddle element?
2. have a separate attrs.diProperties to be applied to the newly created di
3. Require the di to be created before createBpmnElement is called

2 and 3 would be a change to the API, but feel cleaner than overloading the attribute

[nikku]

It is safe to use ModelUtil#is on diagram elements and plain objects. I'd therefor advocate for option 1️⃣.

See also BpmnFactory where we use the utility to verify if an element needs an ID.

[nikku]

Also, we do a similar instanceof if not create check in our core Modeling API already.

[marstamm]

The BpmnReplace will now always create a new di and shape. This is the only place where we checked if it's the same, all other tests just test for an existing new shape as well:

bpmn-js/test/spec/features/modeling/behavior/ReplaceElementBehaviourSpec.js

Lines 474 to 491 in fc2201a

	inject(function(elementRegistry, modeling, elementFactory) {
	// given
	var subProcess = elementRegistry.get('SubProcess_3'),
	startEvent = elementFactory.createShape(
	{ type: 'bpmn:StartEvent', eventDefinitionType: 'bpmn:TimerEventDefinition' }),
	id = startEvent.businessObject.id;
	// when
	modeling.createElements(
	startEvent, { x: subProcess.x + 30, y: subProcess.y + 30 }, subProcess);
	// then
	var createdEvent = elementRegistry.get(id);
	expect(createdEvent).to.exist;
	expect(createdEvent.businessObject.eventDefinitions).to.not.exist;
	})

[nikku]

I've added another minor cleanup on top.

Let us merge this and fix things that are left open later on 🎉.