oozcitak / xmlbuilder-js

An XML builder for node.js
MIT License
919 stars 108 forks source link

multiple importDocument() after begin() removes all other childs #214

Closed danielsetreus closed 5 years ago

danielsetreus commented 5 years ago

On version 13.0.2 - upgraded to get fix for other bug #213 (loved the quick response time and fix btw)

const doc = begin();
const boldDoc = create('b');
boldDoc.text('Hello World');
doc.importDocument(boldDoc);

const emDoc = create('em');
emDoc.text('Hello em');
doc.importDocument(emDoc);

const main = create('p');
main.importDocument(doc);

Expected output:

<p><b>Hello World</b><em>Hello em</em></p>

Actual output:

<p><em>Hello em</em></p>

In this case doc only keeps the last imported document. Documents created with create works as expected.

What is the actual difference between create() and begin()? I've always considered it to be the same, but begin() creates a "blank" root document...?

oozcitak commented 5 years ago

The difference between begin and create is that create returns the document element node, where begin returns the document itself, since it doesn't create an element node. So when you do doc.importDocument you are importing nodes as children of the document node. The second time you call importDocument the document element node b is replaced by em, since an XML document can only have one document element (root) node. If this was allowed the document would be in an invalid state:

const doc = begin();
const boldDoc = create('b');
boldDoc.text('Hello World');
doc.importDocument(boldDoc);
// doc: <b>Hello World</b>
// OK, valid document

const emDoc = create('em');
emDoc.text('Hello em');
doc.importDocument(emDoc);
// expected doc: <b>Hello World</b><em>Hello em</em>
// this would be invalid, so the document element node is replaced
// actual doc: <em>Hello em</em>

const main = create('p');
main.importDocument(doc);
// the document would again be valid at this point, however it doesn't
// survive the previous step

So in that case, importDocument is working as intended.

A solution to your case would be introducing DocumentFragment nodes. We would implement the DocumentFragment from DOM, we would also add a new export function, say fragment for creating a new DocumentFragment node and we would modify importDocument to also accept fragments. This would be quite an undertaking though. I would rather work that into the new iteration of the library since it already supports document fragments through its underlying DOM implementation.

danielsetreus commented 5 years ago

I see, thank you for a great answer. I agree that there is a need for DocumentFragment, but fully understand that you want to focus on xmlbuilder2. Wish you all the best with that project.