Skip to content

Commit

Permalink
Merge pull request #1035 from assetgraph/feature/tryJsdomForXmlAgain
Browse files Browse the repository at this point in the history 
Use jsdom instead of xmldom for XML assets
  • Loading branch information
@papandreou
papandreou committed Aug 17, 2019
2 parents 84cb278 + 16c4fc3 commit d79fdbf
Show file tree
Hide file tree
Showing 16 changed files with 146 additions and 117 deletions.
5 changes: 4 additions & 1 deletion lib/AssetGraph.js
View file
Expand Up @@ -168,7 +168,10 @@ class AssetGraph extends EventEmitter {
if (this.listeners('warn').length > 0) {
this.emit('warn', err);
} else {
err.message = `Encountered warning, add a 'warn' event handler to suppress:\n${err.stack}`;
// jsdom's DOMException throws when attempting to update the message property directly
Object.defineProperty(err, 'message', {
value: `Encountered warning, add a 'warn' event handler to suppress:\n${err.stack}`
});
throw err;
}
}
Expand Down
12 changes: 2 additions & 10 deletions lib/assets/Asset.js
View file
Expand Up @@ -510,16 +510,8 @@ class Asset extends EventEmitter {
}
return this;
} catch (err) {
err.message = err.message || err.code || err.name;
const includingAssetUrls = this.incomingRelations.map(
incomingRelation => {
return incomingRelation.from.urlOrDescription;
}
);
if (includingAssetUrls.length > 0) {
err.message += `\nIncluding assets:\n ${includingAssetUrls.join(
'\n '
)}\n`;
if (!err.message) {
err.message = err.code || err.name;
}
err.asset = this;
throw err;
Expand Down
73 changes: 31 additions & 42 deletions lib/assets/Xml.js
View file
@@ -1,52 +1,34 @@
const DOMParser = require('xmldom').DOMParser;
const errors = require('../errors');
const JSDOM = require('jsdom').JSDOM;
const Text = require('./Text');

class Xml extends Text {
get parseTree() {
if (!this._parseTree) {
let firstParseError;
const domParser = new DOMParser({
errorHandler(err) {
if (Object.prototype.toString.call(err) !== '[object Error]') {
err = new Error(err);
}
firstParseError = firstParseError || err;
}
});
const document = domParser.parseFromString(this.text, 'text/xml');

if (firstParseError) {
const err = new errors.ParseError({
message: `Parse error in ${this.urlOrDescription}\n${firstParseError.message}`,
asset: this
const text = this.text;
const matchXmlDeclaration = text.match(/^<\?[^?>]*?\?>\s*/);
if (matchXmlDeclaration) {
this.xmlDeclaration = matchXmlDeclaration[0];
} else {
this.xmlDeclaration = '';
}
try {
this._jsdom = new JSDOM(text, {
url: this.url ? this.url : undefined, // So that errors get reported with the url (awaiting https://github.com/jsdom/jsdom/pull/2630)
contentType: 'application/xhtml+xml'
});
} catch (err) {
err.asset = this;
if (this.assetGraph) {
this.assetGraph.warn(err);
} else {
throw err;
}
this._jsdom = new JSDOM('<?xml version="1.0"?><root/>', {
url: this.url ? this.url : undefined, // So that errors get reported with the url (awaiting https://github.com/jsdom/jsdom/pull/2630)
contentType: 'application/xhtml+xml'
});
}
if (document) {
// Workaround for https://github.com/jindw/xmldom/pull/59
const fixUpDocTypeNode = doctypeNode => {
if (!doctypeNode || doctypeNode.nodeType !== 10) {
return;
}
for (const doctypePropertyName of ['publicId', 'systemId']) {
if (doctypeNode[doctypePropertyName]) {
doctypeNode[doctypePropertyName] = doctypeNode[
doctypePropertyName
].replace(/"/g, '');
}
}
};
fixUpDocTypeNode(document.doctype);
for (const childNode of Array.from(document.childNodes)) {
fixUpDocTypeNode(childNode);
}
this._parseTree = document;
}
this._parseTree = this._jsdom.window.document;
}
return this._parseTree;
}
Expand All @@ -64,7 +46,7 @@ class Xml extends Text {
get text() {
if (typeof this._text !== 'string') {
if (this._parseTree) {
this._text = this._parseTree.toString();
this._text = `${this.xmlDeclaration}${this._jsdom.serialize()}`;
} else {
this._text = this._getTextFromRawSrc();
}
Expand All @@ -76,6 +58,14 @@ class Xml extends Text {
super.text = text;
}

unload() {
super.unload();
if (this._jsdom) {
this._jsdom.window.close();
this._jsdom = undefined;
}
}

findOutgoingRelationsInParseTree() {
const outgoingRelations = super.findOutgoingRelationsInParseTree();
if (!this.isLoaded) {
Expand All @@ -91,7 +81,7 @@ class Xml extends Text {
}
if (node.nodeType === 7) {
// PROCESSING_INSTRUCTION_NODE
if (node.tagName === 'xml-stylesheet') {
if (node.nodeName === 'xml-stylesheet') {
const matchData = node.data.match(/href="([^"]*)"/);
if (matchData) {
outgoingRelations.push({
Expand Down Expand Up @@ -121,9 +111,8 @@ class Xml extends Text {

prettyPrint() {
this.isPretty = true;
/*eslint-disable*/
const parseTree = this.parseTree; // So markDirty removes this._text
/* eslint-enable */
// eslint-disable-next-line no-unused-expressions
this.parseTree; // So markDirty removes this._text
this.markDirty();
return this;
}
Expand Down
9 changes: 6 additions & 3 deletions lib/relations/HtmlSvgIsland.js
View file
Expand Up @@ -4,10 +4,13 @@ const HtmlRelation = require('./HtmlRelation');
class HtmlSvgIsland extends HtmlRelation {
inline() {
super.inline();
const svgText = Array.from(this.to.parseTree.documentElement.childNodes)
.map(svgElementChildNode => svgElementChildNode.toString())
.join('');

// Get the outerHTML of the documentElement instead of serializing
// the childNodes, avoiding a duplication of xmlns attributes:
const svgText = this.to.parseTree.documentElement.outerHTML.replace(
/^<svg[^>]*>\s*|\s*<\/svg>\n?$/g,
''
);
const isSeenByAttributeName = {};
for (const attribute of Array.from(
this.to.parseTree.documentElement.attributes
Expand Down
8 changes: 8 additions & 0 deletions lib/transforms/logEvents.js
View file
Expand Up @@ -63,6 +63,14 @@ module.exports = ({ afterTransform, repl, stopOnWarning } = {}) => {
}
}
message = `${fileLineCol.join(':')} - ${message}`;
const incomingRelations = messageOrError.asset.incomingRelations;
if (incomingRelations.length > 0) {
message += `\nIncluding assets:\n ${incomingRelations
.map(incomingRelation => {
return incomingRelation.from.urlOrDescription;
})
.join('\n ')}\n`;
}
}
} else {
if (typeof messageOrError === 'string') {
Expand Down
3 changes: 1 addition & 2 deletions package.json
View file
Expand Up @@ -57,8 +57,7 @@
"sw-precache": "^5.2.0",
"teepee": "^2.31.1",
"terser": "^4.0.0",
"urltools": "^0.4.1",
"xmldom": "^0.1.27"
"urltools": "^0.4.1"
},
"devDependencies": {
"autoprefixer": "^9.0.0",
Expand Down
32 changes: 15 additions & 17 deletions test/addAsset.js
View file
Expand Up @@ -202,8 +202,7 @@ describe('AssetGraph#addAsset', function() {
const assetGraph = new AssetGraph();
const xmlAsset = assetGraph.addAsset({
url: 'http://example.com/feed.xml',
text: `
<?xml version="1.0" encoding="utf-8"?>
text: `<?xml version="1.0" encoding="utf-8"?>
<feed xmlns="http://www.w3.org/2005/Atom">
<title>Example blog</title>
<updated>2014-08-29T00:11:13+02:00</updated>
Expand Down Expand Up @@ -245,21 +244,20 @@ describe('AssetGraph#addAsset', function() {
const atomAsset = assetGraph.addAsset({
type: 'Atom',
url: 'http://example.com/feed.xml',
text: `
<?xml version="1.0" encoding="utf-8"?>
<feed xmlns="http://www.w3.org/2005/Atom">
<title>Example blog</title>
<updated>2014-08-29T00:11:13+02:00</updated>
<id>http://example.com/</id>
<entry>
<title>Karma Generator Rewrite 0.8.0</title>
<link href="http://example.com/blog/article/"/>
<updated>2014-05-12T00:00:00+02:00</updated>
<id>http://example.com/blog/article/</id>
<content type="html">This contains an image: &lt;img src=&quot;foo.png&quot;&gt; and a &lt;a href=&quot;bar.html&quot;&gt;relative link&lt;/a&gt;</content>
</entry>
</feed>
`
text: `<?xml version="1.0" encoding="utf-8"?>
<feed xmlns="http://www.w3.org/2005/Atom">
<title>Example blog</title>
<updated>2014-08-29T00:11:13+02:00</updated>
<id>http://example.com/</id>
<entry>
<title>Karma Generator Rewrite 0.8.0</title>
<link href="http://example.com/blog/article/"/>
<updated>2014-05-12T00:00:00+02:00</updated>
<id>http://example.com/blog/article/</id>
<content type="html">This contains an image: &lt;img src=&quot;foo.png&quot;&gt; and a &lt;a href=&quot;bar.html&quot;&gt;relative link&lt;/a&gt;</content>
</entry>
</feed>
`
});

await atomAsset.load();
Expand Down
2 changes: 1 addition & 1 deletion test/assets/Asset.js
View file
Expand Up @@ -1906,7 +1906,7 @@ describe('assets/Asset', function() {
<html>
<head></head>
<body>
<svg><use href="#foo" xlink:href="#foo"></use></svg>
<svg xmlns:xlink="http://www.w3.org/1999/xlink"><use href="#foo" xlink:href="#foo"></use></svg>
</body>
</html>
`
Expand Down
42 changes: 42 additions & 0 deletions test/assets/Xml.js
View file
Expand Up @@ -26,4 +26,46 @@ describe('assets/Xml', function() {
xml.markDirty();
expect(xml.text, 'to match', /foobarquux/);
});

it('should parse a document without an XML declaration', function() {
// eg. svgo omits this
const xmlAsset = new AssetGraph().addAsset({
type: 'Xml',
url: 'https://example.com/',
text: '<doc />\n'
});
expect(() => xmlAsset.parseTree, 'not to throw');
});

describe('#text', function() {
describe('when the original document included an XML declaration', function() {
it('should include the XML declaration when reserializing', function() {
const xmlAsset = new AssetGraph().addAsset({
type: 'Xml',
url: 'https://example.com/',
text: '<?xml version="1.0" encoding="UTF-8"?>\n<doc />'
});
xmlAsset.parseTree; // eslint-disable-line no-unused-expressions
xmlAsset.markDirty();
expect(
xmlAsset.text,
'to equal',
'<?xml version="1.0" encoding="UTF-8"?>\n<doc/>'
);
});
});

describe('when the original document did not include an XML declaration', function() {
it('should not include an XML declaration when reserializing', function() {
const xmlAsset = new AssetGraph().addAsset({
type: 'Xml',
url: 'https://example.com/',
text: '<doc />'
});
xmlAsset.parseTree; // eslint-disable-line no-unused-expressions
xmlAsset.markDirty();
expect(xmlAsset.text, 'to equal', '<doc/>');
});
});
});
});
19 changes: 9 additions & 10 deletions test/relations/HtmlRelation.js
View file
Expand Up @@ -957,22 +957,21 @@ describe('relations/HtmlRelation', function() {
assetGraph.addAsset({
type: 'Svg',
url: 'https://example.com/image.svg',
text: `
<?xml version="1.0" encoding="UTF-8"?>
<svg width="82px" height="90px" viewBox="0 0 82 90" version="1.1" xmlns="http://www.w3.org/2000/svg">
<g id="heart">
<path d="M32,11.2c0,2.7-1.2,5.1-3,6.8l0,0L19,28c-1,1-2,2-3,2s-2-1-3-2L3,18c-1.9-1.7-3-4.1-3-6.8C0,6.1,4.1,2,9.2,2
c2.7,0,5.1,1.2,6.8,3c1.7-1.9,4.1-3,6.8-3C27.9,1.9,32,6.1,32,11.2z"/>
</g>
</svg>
`
text: `<?xml version="1.0" encoding="UTF-8"?>
<svg width="82px" height="90px" viewBox="0 0 82 90" version="1.1" xmlns="http://www.w3.org/2000/svg">
<g id="heart">
<path d="M32,11.2c0,2.7-1.2,5.1-3,6.8l0,0L19,28c-1,1-2,2-3,2s-2-1-3-2L3,18c-1.9-1.7-3-4.1-3-6.8C0,6.1,4.1,2,9.2,2
c2.7,0,5.1,1.2,6.8,3c1.7-1.9,4.1-3,6.8-3C27.9,1.9,32,6.1,32,11.2z"/>
</g>
</svg>
`
});

htmlAsset.outgoingRelations[0].fragment = '#yadda';

htmlAsset.outgoingRelations[0].hrefType = 'inline';

expect(htmlAsset.text, 'to contain', 'AgICAgIA==#yadda">');
expect(htmlAsset.text, 'to contain', 'AgICAgICA=#yadda">');
});
});
});
44 changes: 21 additions & 23 deletions test/relations/MsApplicationConfigPollingUri.js
View file
Expand Up @@ -68,18 +68,17 @@ describe('relations/MsApplicationConfigPollingUri', function() {

const msApplicationConfig = assetGraph.addAsset({
type: 'MsApplicationConfig',
text: `
<?xml version="1.0" encoding="utf-8"?>
<browserconfig>
<msapplication>
<notification>
<frequency>30</frequency>
<polling-uri src="/notification/polling-1.xml"/>
<cycle>1</cycle>
</notification>
</msapplication>
</browserconfig>
`
text: `<?xml version="1.0" encoding="utf-8"?>
<browserconfig>
<msapplication>
<notification>
<frequency>30</frequency>
<polling-uri src="/notification/polling-1.xml"/>
<cycle>1</cycle>
</notification>
</msapplication>
</browserconfig>
`
});

expect(
Expand All @@ -94,17 +93,16 @@ describe('relations/MsApplicationConfigPollingUri', function() {

const msApplicationConfig = assetGraph.addAsset({
type: 'MsApplicationConfig',
text: `
<?xml version="1.0" encoding="utf-8"?>
<browserconfig>
<msapplication>
<notification>
<frequency>30</frequency>
<cycle>1</cycle>
</notification>
</msapplication>
</browserconfig>
`
text: `<?xml version="1.0" encoding="utf-8"?>
<browserconfig>
<msapplication>
<notification>
<frequency>30</frequency>
<cycle>1</cycle>
</notification>
</msapplication>
</browserconfig>
`
});

expect(
Expand Down

0 comments on commit d79fdbf

Please sign in to comment.