bpmn-io / bpmn-moddle

Read and write BPMN 2.0 XML from JavaScript.
MIT License
444 stars 162 forks source link

$parent undefined #92

Closed wiemann closed 2 years ago

wiemann commented 2 years ago

Describe the Bug

When using bpmn-moddle to create elements and adding them to the definitions, the $parent on them is undefined.

When loading an existing process from XML, $parent works on it's in the XML already existing elements as it should. Newly created bpmn-moddle elements have a $parent of undefined.

Steps to Reproduce

  1. Run the example code from the homepage
    
    import BpmnModdle from 'bpmn-moddle';

const moddle = new BpmnModdle();

const xmlStr = '<?xml version="1.0" encoding="UTF-8"?>' + '<bpmn2:definitions xmlns:bpmn2="http://www.omg.org/spec/BPMN/20100524/MODEL" ' + 'id="empty-definitions" ' + 'targetNamespace="http://bpmn.io/schema/bpmn">' + '</bpmn2:definitions>';

const { rootElement: definitions } = await moddle.fromXML(xmlStr);

// update id attribute definitions.set('id', 'NEW ID');

// add a root element const bpmnProcess = moddle.create('bpmn:Process', { id: 'MyProcess_1' }); definitions.get('rootElements').push(bpmnProcess);

// xmlStrUpdated contains new id and the added process const { xml: xmlStrUpdated } = await moddle.toXML(definitions);


2. Inspect the Object `definitions`
![Screenshot 2022-01-21 at 06 43 51](https://user-images.githubusercontent.com/55275/150472792-1dff88b0-58a4-4413-aa14-e76fc1667c98.png)

3. $parent for the newly created Process Element is undefined

__Expected Behavior__

$parent should be a reference to the parent element

__Environment__

- Host: Google Chrome Version 97.0.4692.71 (Official Build) (arm64)
- OS: MacOS Monterey 12.1 (21C52)
- Library version: 7.1.2
pinussilvestrus commented 2 years ago

Thanks for reporting! Giving this API for creating moddle elements

const bpmnProcess = moddle.create('bpmn:Process', { id: 'MyProcess_1' });

what would you expect to be the parent? I think the easiest way would be to make sure you manually set the parent accordingly, to avoid magic operations. Do you see cases, where an automatic parent assignment would make sense?

wiemann commented 2 years ago

The parent should be the definitions element. This works when reading an existing xml: Screenshot 2022-01-21 at 09 56 07

It is not consistent to have the $parent assigned when reading xml vs when modifying it with moddle.

wiemann commented 2 years ago

I have not even found a way to set the $parent manually, probably because of the way it is implemented https://github.com/bpmn-io/moddle-xml/commit/d5e2e833ec02d325d782880848d8d3c4b7d8f874

pinussilvestrus commented 2 years ago

I have not even found a way to set the $parent manually, probably because of the way it is implemented bpmn-io/moddle-xml@d5e2e83

Doesn't it even work for you to just manually set the property?

const bpmnProcess = moddle.create("bpmn:Process", { id: "MyProcess_1" });
bpmnProcess.$parent = definitions;
definitions.get('rootElements').push(bpmnProcess);

image

It is not consistent to have the $parent assigned when reading xml vs when modifying it with moddle.

These are different things. When reading the process via XML, the parent is directly extracted from the XML structure. When just creating it via moddle.create, there is obviously no parent available so far. Yes, given what was previously read (the definitions) one could argue, the created process can only be present as a child of it, and it should be automatically set.

But this is one case, there are more complex ones. As far as I understand the moddle.create can't determine this natural parent in any case, therefore don't set parents automatically.

wiemann commented 2 years ago

All right, setting it manually that way works. I didn't expect the behaviour generally but looking at how the element creation is implemented, obviously it can't determine it's parent.

nikku commented 2 years ago

You have to take care of that yourself right now. It is a limitation you have to be aware of (cf. https://github.com/bpmn-io/moddle/issues/41).