Inserting nodes into a tree

[nyurik]

How would I insert another node into this tree?

<root>
  <elem>abc</elem>
</root>

[Darker]

Same question here.

[nfarina]

xmldoc was originally designed to be a read-only structure. However, I'd be open to exposing an API for modification of the tree if it's relatively simple. I think it's something a lot of people have wanted. If anyone feels like making a PR for this I would be happy to review.

[Darker]

Well, I just added the nodes as raw objects to the existing tree and it worked so far.

Anything more complex would require the node objects to have common ancestor.

[aleph1]

Is there still an interest in this? For a project I am working on I had to add a parent attribute to nodes so I could walk back up the tree after finding a node. So basically this:

function assignParent(xml) {
    xml.children && xml.children.forEach(child => {
        child.parent = xml;
        assignParent(child);
    } );
}

I also had to start to implement some additional functions for manipulating the XML doc with additional methods for the XmlElement, such as prependChild, appendChild, insertBefore, insertAfter, etc.. I tried to stick close to XML DOM as it ends up with more portable code if someone switches to a browser based JS object for XML.

Edit: Though I would be happy to deviate from, or expand upon, the spec, with functions like node.append(...nodes), node.prepend(...nodes), to avoid having to call prependChild or appendChild multiple times.

[nfarina]

I think lots of folks would be happy to see mutability! I think using the spec for inspiration is good since one can carryover their possible knowledge from XML DOM, but I don't think it's necessary to confirm to the spec since the rest of this library doesn't already. And helpful things like (...nodes) are great.

[aleph1]

So, I have this working minus one key aspect, which is what to do with a few properties that are set for each element when the document is parsed and then manipulated. These would be:

XmlElement.line = parser.line;
XmlElement.column = parser.column;
XmlElement.position = parser.position;
XmlElement.startTagPosition = parser.startTagPosition;

My gut would be to invalidate those values once the XML tree has been modified as at that point you are no longer dealing with the original string that was passed to the parser, but really a nested group of objects. Alternatively, if we wanted to maintain those and not destroy them we could add an option to the creation function of an XmlDocument, such as:

function XmlDocument(xml, options) {
  if(options.modifiable) {
  } else {
  }
  ...
}

For xml documents that are modifiable we could always drop the line, column, position, and startTagPosition properties, or simply leave them as undefined. For ones that aren’t modifiable, trying to call any manipulation method (append, prepend, etc.) should probably throw an error.

[aleph1]

You can track the progress on this branch: https://github.com/aleph1/xmldoc/tree/aleph1-node-manipulation

[nfarina]

Well, I'm not sure that invalidating the values "as you go" is going to be great because once you modify the tree, it could have a cascading effect on all elements in the tree. So you'd really have to invalidate the whole tree.

I think your idea of an option in this case is good - but I'd actually make it options.readonly since that name kind of jives with existing JS, and I think the default (if you don't pass this option) should be that the document is not readonly. And in that case, the properties like line/column/etc would just all be null.

If you do pass the readonly option, then line/column/etc would be populated as they are today.

What do you think?

[aleph1]

options.readonly sounds good, but the only issue with having it false by default is that it will be a breaking change if anyone is using the line, column, position, and startTagPosition properties. If you are comfortable with a potentially breaking change by making the default for options.readonly = false I am happy to go that way.

[nfarina]

I'd say let's roll with it in this branch because it feels correct - and when it's ready to merge we can make the call on whether enough other stuff has changed/improved that it warrants a major version bump.

[aleph1]

One other related question. Currently XmlElement.eachChild iterates through children in a forward direction. This can cause an issue if one tries to mutate the children array during iteration. So, this test will fail:

t.test('copy XmlDocument nodes to another XmlDocument', function (t) {

  var items1 = '<items><item id="1" /><item id="2" /></items>';
  var items2 = '<items><item id="3" /><item id="4" /></items>';
  var items1xml = new XmlDocument(items1);
  var items2xml = new XmlDocument(items2);

  items2xml.eachChild(function(child){
    items1xml.append(child);
  });

  t.end();
})

Whereas this one will pass:

t.test('copy XmlDocument nodes to another XmlDocument using while', function (t) {

  var items1 = '<items><item id="1" /><item id="2" /></items>';
  var items2 = '<items><item id="3" /><item id="4" /></items>';
  var items1xml = new XmlDocument(items1);
  var items2xml = new XmlDocument(items2);
  
  while(items2xml.children.length) {
    items1xml.append(items2xml.firstChild);
  }

  t.end();
})

This can be resolved by having XmlElement.eachChild first create a copy of the element’s children var childrenCopy = this.children.slice(); and iterating through the copy. Is that an acceptable change for you? Making that change still results in all of the test passing.

[aleph1]

More updates have been made on that branch. A few notes:

• XmlElement.eachChild now operates on a copy of element.children but all tests still pass
• the 'tag locations' test needed to be modified to include the XmlDocument being instantiated with readonly as true (this testing would not require changing if readonly was true as a default)
• I have started to add tests for the new XmlElement methods

The only glaring issue is going to be how we handle cases where there is an attempt to append an XmlDocument to an XmlElement. In theory this is possible, but it gets weird if someone has created the two XmlDocuments with different settings. Since we can’t detach nodes from a document that is readonly, I am proposing I also add a clone() method that accepts a deep option. Once a node is cloned it loses all of its metadata pertaining to the parser state.

For example:

var readOnlyXmlDoc = new XmlDocument('<items><item id="1"/><item id="2"/></items>');
var writeableXmlDoc = new XmlDocument('<items />');

// this approach wouldn’t work
readOnlyXmlDoc.eachChild(function(node) {
  writeableXmlDoc.append(node); // error throws here
});

// this approach would work
readOnlyXmlDoc.eachChild(function(node) {
  writeableXmlDoc.append(node.clone());
});

[aleph1]

I've added the following to this branch: https://github.com/aleph1/xmldoc/tree/aleph1-node-manipulation

• Clone support for each node type
• Empty method for XmlElement
• Modified how XmlElement is created to allow for manual creation outside of the context of new XmlDocument
• Added additional tests
  • manually creating nodes of different types
  • cloning nodes
  • moving elements from one node to another
  • prepending nodes
  • (more to come)

A few more comments and questions:

• Currently XmlElement.val is modified in two places, XmlElement.prototype._text, and XmlElement.prototype._cdata, but with the new API val will be inconsistent when a node’s children is modified using append, prepend, etc.. I think we should move modifying .val to within the methods that actually modify the node’s children, as anytime children is modified .val will need to be recalculated.
• Right now direct modification of a node’s .children array will break walking up the tree as the inserted child will not have .parent set. If we moved to using Proxy we could avoid this issue by preventing direct manipulation of the children array. Are we okay with this implementation? It could be flagged in the documentation.
• Like the previous point there are similar issues with modifying XmlTextNode.text directly. For example, if an XmlElement has a XmlTextNode as a child, and XmlTextNode.text is modified then the parent XmlElement.val will be inaccurate. Should we creating an API for all element types? Such as XmlTextNode.setText(), XmlTextNode.getText(), or should we consider using getters and setters and dropping support for some older browsers?
• There are probably another dozen or so tests to write. I will try to get to them over the next few days. I know there will be edge cases to catch.
  • One of these cases will be what happens an XmlDocument is made a child of an XmlElement. This works at present because of the nature of the code, but it raises some issues. Should we allow it? Emit a warning?

[nfarina]

This is all fantastic. My thoughts:

1. I think copying the array for eachChild is correct as it mimics Array.forEach
2. Love clone()!
3. I think attempting to add an XmlDocument to another XmlDocument or XmlElement should throw an Error, it just doesn't make sense especially because of the parser objects attached to XmlDocument
4. Not totally following the val situation - is the problem that allowing direct setting of the .val property creates problems? If so I think making folks use a setter would be fine? I definitely think being able to say authorNode.val = "Trudy Price" is better than having to create and append a new text node
5. I think Proxy could be a clever approach to protecting children - worth trying, and if it feels clunky we can just say, "don't modify children directly" in the docs, or perhaps make children a getter that returns a copy of the array?
6. I think getters and setters are fine to use. I do want to continue support older browsers/node as I feel like that's this library's main selling point, but getters and setters are pretty old by now!

[aleph1]

3. I will have this throw an error. Though I might recommend an XmlDocument.toXmlElement function that removes this extraneous data, or a XmlDocument.cloneAsXmlElement function so the original XmlDocument is maintained. This could be useful if someone wants to attach one document to another without having to write a fair bit of code.
4. In the current code .val is updated as elements are appended to an XmlElement instance, and simply uses +=. Since we are going to allow modification of children then I need to basically recalculate .val any time the children array changes. Does this provide enough clarification?
5. I will implement Proxy in such a way that it will be easy to remove if we decide against it. So much is changing that if this is going to be a major version bump we could also convert element.children to suggest privacy and rename it element._children. Thoughts?
6. Okay.

I am aiming to have these changes in place by next week.

[nfarina]

3. I like XmlDocument.cloneAsElement(), that sounds pretty clear!
4. Makes sense. Maybe there's a _val internal in the same way as _children that discourages direct modification.

[StephenBurroughs]

Is there any intention to merge this mutable functionality into the main branch?