Skip to content

Conversation

@laubstein
Copy link
Contributor

This commit fixes #33 .

The code supports only version, encoding and standalone xml prolog attributes (here).
The new special attribute (xmlProlog) is using the default attrPrefix, is this ok?

I have added two new configs:

forceXmlProlog: false,
defaultXmlProlog: '<?xml version="1.0" encoding="UTF-8"?>'

Unit test

  describe('xml prolog', function() {
    it('should be consistent', function() {
      var originalString = '<?xml version="1.0"?><a>1</a>';
      var xmlObject = JXON.stringToXml(originalString);
      var jsonFromXml = JXON.xmlToJs(xmlObject);
      var jsonFromString = JXON.stringToJs(originalString);

      assert.equal(originalString, JXON.jsToString(jsonFromXml));
      assert.equal(originalString, JXON.jsToString(jsonFromString));

      // PhantomJS do not serialize the prolog, so we are forcing a default
      JXON.config({
        forceXmlProlog: true,
        defaultXmlProlog: '<?xml version="1.0"?>'
      });
      assert.equal(originalString, JXON.xmlToString(xmlObject));
    });
  });

The new JSON representation from

<?xml version="1.0"?>
<a>foo</a>

will be...

{
    "a": "foo",
    "$xmlProlog": {
        "version":"1.0"
    }
}

var rIsNull = /^\s*$/;
var rIsBool = /^(?:true|false)$/i;
var rXmlProlog = /^(<\?xml.*?\?>[\n]?)/;
var rXmlPrologAttributes = /\b(version|encoding|standalone)="([^"]+?)"/g;
Copy link
Owner

Choose a reason for hiding this comment

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

these attributes could also be quoted in single quotes ' according to the specs

Copy link
Contributor Author

@laubstein laubstein Aug 17, 2016

Choose a reason for hiding this comment

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

My mistake... that's easy to fix.

@tyrasd
Copy link
Owner

tyrasd commented Aug 17, 2016

supports only version, encoding and standalone

that's ok, because these are the only three allowed pseudo-attributes of the xml declaration (see https://www.w3.org/TR/2004/REC-xml-20040204/#sec-prolog-dtd)

using the default attrPrefix

Yeah, that's fine, because the top-most level in the JXON representation cannot currently have any attributes, as it would reference the "parent" of the xml root element. The analogy that this should apply to the pseudo-attributes of the xml document suggests itself.

But I think your current implementation is a bit over-complicated, if you ask me. What do you think about the following proposals:

  • I think the more commonly used name for the xml prolog is xml declaration, let's use a nomenclature that's a bit closer to that.
  • You're adding a couple of helpers only to parse something that's than immediately stringified again. What if we screw all that and just save everything that matches the xml declaration regexp into a string (i.e. as if it were a full xml node)? That would save us a lot of code and for also most (or all?) of the problems I've highlighted in the PR.
  • make forceXmlProlog default to true.

@laubstein
Copy link
Contributor Author

I will try to fix all points and them submit a new commit. 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

XML Prolog inconsistence

2 participants