Fix timeouts by making inner loop faster (#339)

* Remove dumb nodeWalkPrior from collecting children of `<head>` We only really need to check the childnodes of `<head>`, `queryAll` was too broad, and checking each node for being inside of a `<template>` is both slow and unnecessary. Fixes #336 * Minimize leftover whitespace and vulcanize-hidden divs * Switch to eslint Fix lint errors found with eslint * more restrictive check for console
Polymer · Apr 21, 2016 · 4511f01 · 4511f01
1 parent c81e859
commit 4511f01
Show file tree

Hide file tree

Showing 9 changed files with 80 additions and 47 deletions.
diff --git a/.eslintignore b/.eslintignore
@@ -0,0 +1 @@
+test/html/*
diff --git a/.eslintrc.json b/.eslintrc.json
@@ -0,0 +1,6 @@
+{
+  "extends": "eslint:recommended",
+  "env": {
+    "node": true
+  }
+}
diff --git a/.jshintignore b/.jshintignore
diff --git a/bin/.eslintrc.json b/bin/.eslintrc.json
@@ -0,0 +1,5 @@
+{
+  "rules": {
+    "no-console": "off"
+  }
+}
diff --git a/lib/matchers.js b/lib/matchers.js
@@ -86,5 +86,5 @@ module.exports = {
   POLY_CSS_LINK: polymerExternalStyle,
   ALL_CSS_LINK: p.OR(externalStyle, polymerExternalStyle),
   JS_SRC: p.AND(p.hasAttr('src'), jsMatcher),
-  JS_INLINE: p.AND(p.NOT(p.hasAttr('src')), jsMatcher),
+  JS_INLINE: p.AND(p.NOT(p.hasAttr('src')), jsMatcher)
 };
diff --git a/lib/vulcan.js b/lib/vulcan.js
@@ -110,7 +110,7 @@ var Vulcan = function Vulcan(opts) {
 
 Vulcan.prototype = {
   isDuplicateImport: function isDuplicateImport(importMeta) {
-    return !Boolean(importMeta.href);
+    return !importMeta.href;
   },
 
   reparent: function reparent(newParent) {
@@ -153,19 +153,19 @@ Vulcan.prototype = {
     return Boolean(dom5.query(doc, matchers.polymerElement));
   },
 
-  // when removing imports, remove the newline after it as well
-  removeImportAndNewline: function removeImportAndNewline(importNode, replacement) {
-    var parent = importNode.parentNode;
-    var nextIdx = parent.childNodes.indexOf(importNode) + 1;
+  removeElementAndNewline: function removeElementAndNewline(node, replacement) {
+    // when removing nodes, remove the newline after it as well
+    var parent = node.parentNode;
+    var nextIdx = parent.childNodes.indexOf(node) + 1;
     var next = parent.childNodes[nextIdx];
     // remove next node if it is blank text
     if (this.isBlankTextNode(next)) {
       dom5.remove(next);
     }
     if (replacement) {
-      dom5.replace(importNode, replacement);
+      dom5.replace(node, replacement);
     } else {
-      dom5.remove(importNode);
+      dom5.remove(node);
     }
   },
 
@@ -176,6 +176,26 @@ Vulcan.prototype = {
     return false;
   },
 
+  moveToBodyMatcher: dom5.predicates.AND(
+    dom5.predicates.OR(
+      dom5.predicates.hasTagName('script'),
+      dom5.predicates.hasTagName('link')
+    ),
+    dom5.predicates.NOT(
+      matchers.polymerExternalStyle
+    )
+  ),
+
+  ancestorWalk: function(node, target) {
+    while(node) {
+      if (node === target) {
+        return true;
+      }
+      node = node.parentNode;
+    }
+    return false;
+  },
+
   flatten: function flatten(tree, isMainDoc) {
     var doc = tree.html.ast;
     var imports = tree.imports;
@@ -188,31 +208,26 @@ Vulcan.prototype = {
     }
     this.fixFakeExternalScripts(doc);
     this.pathResolver.acid(doc, tree.href);
-    var moveTarget = dom5.constructors.fragment();
-    var moveToBody = dom5.queryAll(head,
-      dom5.predicates.AND(
-        dom5.predicates.OR(
-          dom5.predicates.hasTagName('script'),
-          dom5.predicates.hasTagName('link')
-        ),
-        dom5.predicates.NOT(
-          matchers.polymerExternalStyle
-        )
-      )
-    );
-    moveToBody.forEach(function(m) {
-      // don't move nodes if inside a <template> content document
-      if (!dom5.nodeWalkPrior(m, dom5.isDocumentFragment)) {
-        dom5.append(moveTarget, m);
-      }
-    });
+    var moveTarget;
+    if (!isMainDoc) {
+      moveTarget = dom5.constructors.fragment();
+    } else {
+      // hide bodies of imports from rendering in main document
+      moveTarget = dom5.constructors.element('div');
+      dom5.setAttribute(moveTarget, 'hidden', '');
+      dom5.setAttribute(moveTarget, 'by-vulcanize', '');
+    }
+    head.childNodes.filter(this.moveToBodyMatcher).forEach(function(n) {
+      this.removeElementAndNewline(n);
+      dom5.append(moveTarget, n);
+    }, this);
     this.prepend(body, moveTarget);
     if (imports) {
       for (var i = 0, im, thisImport; i < imports.length; i++) {
         im = imports[i];
         thisImport = importNodes[i];
         if (this.isDuplicateImport(im) || this.isStrippedImport(im)) {
-          this.removeImportAndNewline(thisImport);
+          this.removeElementAndNewline(thisImport);
           continue;
         }
         if (this.isExcludedImport(im)) {
@@ -227,34 +242,37 @@ Vulcan.prototype = {
         // merge head and body tags for imports into main document
         var importHeadChildren = importHead.childNodes;
         var importBodyChildren = importBody.childNodes;
-        // make sure @license comments from import document make it into head
+        // make sure @license comments from import document make it into the import
         var importHtml = importHead.parentNode;
         var licenseComments = importDoc.childNodes.concat(importHtml.childNodes).filter(this.isLicenseComment);
-        // put license comments at the top of <head>
-        importHeadChildren = licenseComments.concat(importHeadChildren);
         // move children of <head> and <body> into importer's <body>
         var reparentFn = this.reparent(bodyFragment);
         importHeadChildren.forEach(reparentFn);
         importBodyChildren.forEach(reparentFn);
         bodyFragment.childNodes = bodyFragment.childNodes.concat(
+          licenseComments,
           importHeadChildren,
           importBodyChildren
         );
-        // hide bodies of imports from rendering in main document
-        if (isMainDoc) {
+        // hide imports in main document, unless already hidden
+        if (isMainDoc && !this.ancestorWalk(thisImport, moveTarget)) {
           this.hide(thisImport);
         }
-        this.removeImportAndNewline(thisImport, bodyFragment);
+        this.removeElementAndNewline(thisImport, bodyFragment);
       }
     }
+    // If hidden node is empty, remove it
+    if (isMainDoc && moveTarget.childNodes.length === 0) {
+      dom5.remove(moveTarget);
+    }
     return doc;
   },
 
   hide: function(node) {
     var hidden = dom5.constructors.element('div');
     dom5.setAttribute(hidden, 'hidden', '');
     dom5.setAttribute(hidden, 'by-vulcanize', '');
-    this.removeImportAndNewline(node, hidden);
+    this.removeElementAndNewline(node, hidden);
     dom5.append(hidden, node);
   },
 
@@ -400,7 +418,6 @@ Vulcan.prototype = {
       return analyzer.metadataTree(target);
     }).then(function(tree) {
       var flatDoc = this.flatten(tree, true);
-      var body = dom5.query(flatDoc, matchers.body);
       // make sure there's a <meta charset> in the page to force UTF-8
       var meta = dom5.query(flatDoc, matchers.meta);
       var head = dom5.query(flatDoc, matchers.head);

diff --git a/package.json b/package.json
@@ -15,12 +15,12 @@
   },
   "devDependencies": {
     "chai": "^3.4.1",
+    "eslint": "^2.8.0",
     "firebase": "^2.4.1",
-    "jshint": "^2.7.0",
     "mocha": "^2.2.4"
   },
   "scripts": {
-    "test": "jshint --verbose lib bin/vulcanize test && mocha"
+    "test": "eslint lib bin test && mocha"
   },
   "author": "The Polymer Project Authors",
   "license": "BSD-3-Clause",

diff --git a/test/.eslintrc.json b/test/.eslintrc.json
@@ -0,0 +1,8 @@
+{
+  "env": {
+    "mocha": true
+  },
+  "globals": {
+    "assert": true
+  }
+}
diff --git a/test/test.js b/test/test.js
@@ -96,7 +96,7 @@ suite('Path Resolver', function() {
       '}',
       'x-quuz {',
       '  background-image: url(\'https://foo.bar/baz.jpg\');',
-      '}',
+      '}'
     ].join('\n');
 
     var expected = [
@@ -108,7 +108,7 @@ suite('Path Resolver', function() {
       '}',
       'x-quuz {',
       '  background-image: url("https://foo.bar/baz.jpg");',
-      '}',
+      '}'
     ].join('\n');
 
     var actual = pathresolver.rewriteURL(inputPath, outputPath, css);
@@ -272,7 +272,6 @@ suite('Vulcan', function() {
   var inputPath = path.resolve('test/html/default.html');
 
   var preds = dom5.predicates;
-  var hyd = require('hydrolysis');
   var doc;
 
   function process(inputPath, cb, vulcanizeOptions) {
@@ -304,7 +303,7 @@ suite('Vulcan', function() {
       });
     });
 
-    test('imports were deduplicated', function() {
+    test('imports were deduplicated', function(done) {
       process(inputPath, function(err, doc) {
         if (err) {
           return done(err);
@@ -355,7 +354,7 @@ suite('Vulcan', function() {
       });
     });
 
-    test('output file is forced utf-8', function() {
+    test('output file is forced utf-8', function(done) {
       var meta = preds.AND(
         preds.hasTagName('meta'),
         preds.hasAttrValue('charset', 'UTF-8')
@@ -385,7 +384,7 @@ suite('Vulcan', function() {
         var spanHref = dom5.query(doc, span);
         assert.ok(spanHref);
         var anchorRef = dom5.query(doc, a);
-        assert.ok(a);
+        assert.ok(anchorRef);
         done();
       });
     });
@@ -436,7 +435,7 @@ suite('Vulcan', function() {
     test('Old Polymer is detected and warns', function(done) {
       var constants = require('../lib/constants');
       var input = path.resolve('test/html/old-polymer.html');
-      process(input, function(err, doc) {
+      process(input, function(err) {
         if (err) {
           try {
             // check err message
@@ -679,7 +678,6 @@ suite('Vulcan', function() {
         var imports = dom5.queryAll(doc, htmlImport);
         assert.equal(imports.length, 2);
         var badCss = dom5.queryAll(doc, cssFromExclude);
-        console.log(badCss[0].attrs);
         assert.equal(badCss.length, 0);
         done();
       };