Move to w3c-xmlserializer #2282
Conversation
Sebmaster
force-pushed
the
feature/xmlserializer
branch
from
722baa9
to
3a800cd
Compare
|
@Sebmaster mind taking a look at the test failures? Probably all just bad tests... |
|
I think some of these were because serializing a sub-element always included an xmlns attribute and I wasn't sure if that was a spec bug and how browsers behave in that regard. Will test that out on the weekend. |
This is pending as a PR and hopefully we can remove the extra npm dependency once it is in: jsdom/jsdom#2282
Sebmaster
force-pushed
the
feature/xmlserializer
branch
4 times, most recently
from
884b9a1
to
5371882
Compare
Sebmaster
force-pushed
the
feature/xmlserializer
branch
7 times, most recently
from
d412cc0
to
4641424
Compare
| function htmlSerialization(node) { | ||
| if ( | ||
| node.nodeType === NODE_TYPE.ELEMENT_NODE && | ||
| node.namespaceURI === "http://www.w3.org/1999/xhtml" && |
There was a problem hiding this comment.
This line could use the constant from https://github.com/jsdom/jsdom/blob/master/lib/jsdom/living/helpers/namespaces.js
lib/api.js
Outdated
| @@ -61,7 +61,7 @@ class JSDOM { | |||
| } | |||
|
|
|||
| serialize() { | |||
| return domToHtml([idlUtils.implForWrapper(this[window]._document)]); | |||
| return fragmentSerialization(idlUtils.implForWrapper(this[window]._document), false); | |||
There was a problem hiding this comment.
Can we use an options object instead? https://ariya.io/2011/08/hall-of-api-shame-boolean-trap
This finally gives us proper serialization for XML documents and the XMLSerializer interface.
Sebmaster
force-pushed
the
feature/xmlserializer
branch
from
bd524b3
to
a189c41
Compare
|
Fixed feedback and rebased on master. Greeeen 🌈 |
|
Really looking forward to this, great work! Is it intentional that const JSDOM = require('jsdom').JSDOM;
const text = `<?xml version="1.0" encoding="utf-8"?><foo></foo>`;
const jsdom = new JSDOM(text, {
contentType: 'application/xhtml+xml'
});
const document = jsdom.window.document;
console.log(jsdom.serialize(document)); // <foo/>I can easily work around that, but just wanted to check ;) |
|
Hmm, there is no spec for serialize(), so we get to make up the rules. I tend to think it should be included? I'll try to do that on my next jsdom weekend (and then merge this), unless anyone has counterobjections. |
|
Thanks for the reply! For my case the most handy thing would be if the XML declaration were included if and only if it was in the string that was originally parsed, similar to how const JSDOM = require('jsdom').JSDOM;
function roundTrip(text, contentType = 'text/html') {
const jsdom = new JSDOM(text, { contentType });
console.log(jsdom.serialize(jsdom.window.document));
}
roundTrip(`<html><head></head><body></body></html>`);
// <html><head></head><body></body></html>
roundTrip(`<!DOCTYPE html><html><head></head><body></body></html>`);
// <!DOCTYPE html><html><head></head><body></body></html>
roundTrip('<foo/>', 'application/xml');
// <foo/>
roundTrip('<?xml version="1.0"?><foo/>', 'application/xml');
// <foo/>Would be great if the last one logged |
|
After some investigation it seems that the XML declaration is properly treated as non-existent in the DOM; it's basically a sort of invisible comment/header that identifies the file, and not part of the document object model. Thus, any serializations of the DOM must not include it. As such, this is ready to merge as-is. |
This finally gives us proper serialization for XML documents and the
XMLSerializer interface.