Repository: Polymer/vulcanize (js)

Issue:
vulcanize alters script execution order unexpectedly
vulcanize (1.12.3) unexpectedly alters the order of script execution if an imported document has a script in the `<body>`. Example: http://pastebin.com/raw.php?i=1BPLc5iA

The unvulcanized `in.html`will run the first script first, but loading the output of `vulcanize in.html` will run the second script first.

Is this expected behavior, or a bug?

Pull request:
Fix ordering of import dependencies once and for all
All import portions, `<head>` and `<body>` will go into the body of the main
document. This is the only way to ensure that intra-import dependencies and
inter-import dependencies line up correctly.

Fixes #284

Patch:
diff --git a/README.md b/README.md
index b47037c..525b0c2 100644
--- a/README.md
+++ b/README.md
@@ -25,7 +25,7 @@ for this step).
 - `-h`|`--help`: print this message
 - `-v`|`--version`: print version number
 - `-p <arg>`|`--abspath <arg>`: use <arg> as the "webserver root", make all adjusted urls absolute
-- `--exclude <path>`: exclude a subpath from root. Use multiple times to exclude multiple paths. Tags (imports/scripts/etc) that reference an excluded path are left in-place, meaning the resources are not inlined. ex: `--exclude=elements/x-foo.html --exclude=elements/x-bar.html`
+- `--exclude <path>`: exclude a subpath from root. Use multiple times to exclude multiple paths. Tags to excluded paths are kept.
 - `--strip-exclude`: Exclude a subpath and remove any links referencing it.
 - `--inline-scripts`: Inline external scripts.
 - `--inline-css`: Inline external stylesheets.
@@ -95,7 +95,7 @@ Vulcanize as a library has two exported function.
 options.
 - `abspath`: A folder to treat as "webroot".
   - When specified, use an absolute path to `target`.
-- `excludes`: An array of strings with regular expressions to exclude paths from being inlined.
+- `excludes`: An array of RegExp objects to exclude paths from being inlined.
 - `stripExcludes`: Similar to `excludes`, but strips the imports from the output entirely.
     - If `stripExcludes` is empty, it will be set the value of `excludes` by default.
 - `inlineScripts`: Inline external scripts.
@@ -128,7 +128,6 @@ var loader = new hydrolysis.loader(...)
 var vulcan = new Vulcanize({
   abspath: '',
   excludes: [
-    '\\.css$'
   ],
   stripExcludes: [
   ],
@@ -149,6 +148,15 @@ vulcan.process(target, function(err, inlinedHtml) {
 });
 ```
 
+## Caveats
+
+Because HTML Imports changes the order of execution scripts can have, Vulcanize
+has to make a few compromises to achieve that same script execution order.
+
+1. Contents of all HTML Import documents will be moved to `<body>`
+
+1. Any scripts after a `<link rel="import">` node in `<head>` will be moved to `<body>` after the contents of the HTML Import.
+
 ## What happened to [feature] from 0.X?
 - `--csp` mode has been moved into [crisper](https://github.com/PolymerLabs/crisper)
 - `--strip` mode was removed, use something like [html-minifier](https://github.com/kangax/html-minifier) or [minimize](https://github.com/Moveo/minimize)
diff --git a/lib/vulcan.js b/lib/vulcan.js
index 9509d08..0fd0c0d 100644
--- a/lib/vulcan.js
+++ b/lib/vulcan.js
@@ -195,17 +195,20 @@ Vulcan.prototype = {
     }
     this.pathResolver.acid(doc, tree.href);
     if (imports) {
-      for (var i = 0, im; i < imports.length; i++) {
+      // flatten all of <head> into one list
+      var flatHead = dom5.queryAll(head, function(){ return true; });
+      for (var i = 0, im, thisImport; i < imports.length; i++) {
         im = imports[i];
+        thisImport = importNodes[i];
         if (this.isDuplicateImport(im)) {
-          this.removeImportAndNewline(importNodes[i]);
+          this.removeImportAndNewline(thisImport);
           continue;
         }
         if (this.isExcludedImport(im)) {
           continue;
         }
         if (this.isStrippedImport(im)) {
-          this.removeImportAndNewline(importNodes[i]);
+          this.removeImportAndNewline(thisImport);
           continue;
         }
         var importDoc = this.flatten(im, bodyFragment, mainDocUrl);
@@ -216,8 +219,14 @@ Vulcan.prototype = {
         // merge head and body tags for imports into main document
         var importHeadChildren = importHead.childNodes;
         var importBodyChildren = importBody.childNodes;
-        // replace link in head with head elements from import
-        this.replaceWith(head, importNodes[i], importHeadChildren);
+        // delete link
+        this.removeImportAndNewline(thisImport);
+        // defer head children to be ordered properly in main document
+        if (importHeadChildren.length) {
+          this.pathResolver.resolvePaths(importHead, tree.href, mainDocUrl);
+          importHeadChildren.forEach(this.reparent(bodyFragment));
+          bodyFragment.childNodes = bodyFragment.childNodes.concat(importHeadChildren);
+        }
         // defer body children to be inlined in-order
         if (importBodyChildren.length) {
           // adjust body urls to main document
@@ -225,27 +234,29 @@ Vulcan.prototype = {
           importBodyChildren.forEach(this.reparent(bodyFragment));
           bodyFragment.childNodes = bodyFragment.childNodes.concat(importBodyChildren);
         }
-      }
-    }
-    if (this.stripComments) {
-      var comments = new CommentMap();
-      // remove all comments
-      dom5.nodeWalkAll(doc, dom5.isCommentNode).forEach(function(comment) {
-        comments.set(comment.data, comment);
-        dom5.remove(comment);
-      });
-      // Deduplicate license comments
-      comments.keys().forEach(function (commentData) {
-        if (commentData.indexOf("@license") == -1) {
-          return;
+        // to preserve script execution order, defer scripts after this import
+        // into body
+        var importInBody = Boolean(dom5.nodeWalkPrior(thisImport, matchers.body));
+        if (!importInBody) {
+          // only examine elements between this and the next import
+          var start = flatHead.indexOf(thisImport) + 1;
+          var end = flatHead.indexOf(importNodes[i + 1]);
+          if (end === -1) {
+            end = flatHead.length;
+          }
+          for (var headidx = start, el; headidx < end; headidx++) {
+            el = flatHead[headidx];
+            // if the element is a script, move it to bodyFragment
+            if (matchers.JS(el)) {
+              dom5.append(bodyFragment, el);
+            }
+          }
         }
-        this.prepend(head, comments.get(commentData));
-      }.bind(this));
+      }
     }
     return doc;
   },
 
-
   prepend: function prepend(parent, node) {
     if (parent.childNodes.length) {
       dom5.insertBefore(parent, parent.childNodes[0], node);
@@ -391,6 +402,26 @@ Vulcan.prototype = {
         return this.inlineCss(docObj.doc, docObj.href);
       }.bind(this));
     }
+    if (this.stripComments) {
+      chain = chain.then(function(docObj) {
+        var comments = new CommentMap();
+        var doc = docObj.doc;
+        var head = dom5.query(doc, matchers.head);
+        // remove all comments
+        dom5.nodeWalkAll(doc, dom5.isCommentNode).forEach(function(comment) {
+          comments.set(comment.data, comment);
+          dom5.remove(comment);
+        });
+        // Deduplicate license comments
+        comments.keys().forEach(function (commentData) {
+          if (commentData.indexOf("@license") == -1) {
+            return;
+          }
+          this.prepend(head, comments.get(commentData));
+        }, this);
+        return docObj;
+      }.bind(this));
+    }
     chain.then(function(docObj) {
       cb(null, dom5.serialize(docObj.doc));
     }).catch(cb);


Test patch:
diff --git a/test/html/comments.html b/test/html/comments.html
index 9393449..7d72687 100644
--- a/test/html/comments.html
+++ b/test/html/comments.html
@@ -1,4 +1,5 @@
 <!DOCTYPE html>
+<!-- @license test -->
 <html lang="en">
 <head>
   <meta charset="UTF-8">
diff --git a/test/html/imports/body-import.html b/test/html/imports/body-import.html
index 51e14f8..a3e21cc 100644
--- a/test/html/imports/body-import.html
+++ b/test/html/imports/body-import.html
@@ -11,6 +11,6 @@
 <script>
   'expected in head'
 </script>
-<div>
+<div id="imported">
   expected in body
 </div>
diff --git a/test/html/imports/comment-in-import.html b/test/html/imports/comment-in-import.html
index 2c1e48f..f6db123 100644
--- a/test/html/imports/comment-in-import.html
+++ b/test/html/imports/comment-in-import.html
@@ -1,2 +1,7 @@
+<!-- @license test -->
 <link rel="import" href="sub/second.html">
 <!-- comment in import -->
+<div>
+  <!-- @license test -->
+  <!-- comment in import -->
+</div>
diff --git a/test/html/order-test.html b/test/html/order-test.html
new file mode 100644
index 0000000..9fb8820
--- /dev/null
+++ b/test/html/order-test.html
@@ -0,0 +1,13 @@
+<!DOCTYPE html>
+<html lang="en">
+<head>
+  <meta charset="UTF-8">
+  <title></title>
+  <script id="first-script" src="order/first-script.js"></script>
+  <link rel="import" href="order/first-import.html">
+  <script id="second-script" src="order/second-script.js"></script>
+</head>
+<body>
+  <script id="third-script" src="order/third-script.js"></script>
+</body>
+</html>
diff --git a/test/html/order/first-import.html b/test/html/order/first-import.html
new file mode 100644
index 0000000..dc106e1
--- /dev/null
+++ b/test/html/order/first-import.html
@@ -0,0 +1,3 @@
+<script id="first-import-script">console.log('first-import-script')</script>
+<div></div>
+<script id="second-import-script">console.log('second-import-script')</script>
diff --git a/test/html/order/first-script.js b/test/html/order/first-script.js
new file mode 100644
index 0000000..6c94ccf
--- /dev/null
+++ b/test/html/order/first-script.js
@@ -0,0 +1,1 @@
+console.log('first-script');
diff --git a/test/html/order/second-script.js b/test/html/order/second-script.js
new file mode 100644
index 0000000..53c59e8
--- /dev/null
+++ b/test/html/order/second-script.js
@@ -0,0 +1,1 @@
+console.log('second-script');
diff --git a/test/html/order/third-script.js b/test/html/order/third-script.js
new file mode 100644
index 0000000..540b8d5
--- /dev/null
+++ b/test/html/order/third-script.js
@@ -0,0 +1,1 @@
+console.log('third-script');
diff --git a/test/html/reordered/first.html b/test/html/reordered/first.html
new file mode 100644
index 0000000..50748fb
--- /dev/null
+++ b/test/html/reordered/first.html
@@ -0,0 +1,8 @@
+<!doctype html>
+<html>
+<head>
+</head>
+<body>
+<script>"First"</script>
+</body>
+</html>
diff --git a/test/html/reordered/in.html b/test/html/reordered/in.html
new file mode 100644
index 0000000..9011446
--- /dev/null
+++ b/test/html/reordered/in.html
@@ -0,0 +1,8 @@
+<!doctype html>
+<html>
+<head></head>
+<body>
+  <link rel="import" href="first.html">
+  <link rel="import" href="second.html">
+</body>
+</html>
diff --git a/test/html/reordered/second.html b/test/html/reordered/second.html
new file mode 100644
index 0000000..401519c
--- /dev/null
+++ b/test/html/reordered/second.html
@@ -0,0 +1,1 @@
+<script>"Second"</script>
\ No newline at end of file
diff --git a/test/test.js b/test/test.js
index 59ad0de..a9bde7a 100644
--- a/test/test.js
+++ b/test/test.js
@@ -363,11 +363,10 @@ suite('Vulcan', function() {
         preds.hasAttrValue('rel', 'import')
       );
 
-      var headMatcher = preds.hasTagName('head');
-      var bodyMatcher = preds.hasTagName('body');
+      var bodyContainerMatcher = preds.AND(preds.hasTagName('div'), preds.hasAttr('hidden'), preds.hasAttr('by-vulcanize'));
 
-      var headExpected = preds.hasTagName('script');
-      var bodyExpected = preds.hasTagName('div');
+      var scriptExpected = preds.hasTagName('script');
+      var divExpected = preds.AND(preds.hasTagName('div'), preds.hasAttrValue('id', 'imported'));
 
       process('test/html/import-in-body.html', function(err, doc) {
         if (err) {
@@ -375,16 +374,40 @@ suite('Vulcan', function() {
         }
         var imports = dom5.queryAll(doc, importMatcher);
         assert.equal(imports.length, 0);
-        var head = dom5.query(doc, headMatcher);
-        var body = dom5.query(doc, bodyMatcher);
-        var headActual = dom5.query(doc, headExpected).parentNode;
-        var bodyActual = dom5.query(doc, bodyExpected).parentNode;
-        assert.equal(head, headActual);
-        assert.equal(body, bodyActual);
+        var bodyContainer = dom5.query(doc, bodyContainerMatcher);
+        var scriptActual = dom5.query(doc, scriptExpected).parentNode;
+        var divActual = dom5.query(doc, divExpected).parentNode;
+        assert.equal(bodyContainer, scriptActual);
+        assert.equal(bodyContainer, divActual);
         done();
       });
     });
 
+    test('Imports and scripts are ordered correctly', function(done) {
+      var expected = [
+        'first-script',
+        'first-import-script',
+        'second-import-script',
+        'second-script',
+        'third-script'
+      ];
+
+      var scriptMatcher = preds.hasTagName('script');
+
+      var callback = function(err, doc) {
+        if (err) {
+          return done(err);
+        }
+        var scripts = dom5.queryAll(doc, scriptMatcher).map(function(s) {
+          return dom5.getAttribute(s, 'id');
+        });
+        assert.deepEqual(scripts, expected);
+        done();
+      };
+
+      process('test/html/order-test.html', callback);
+    });
+
     test('Old Polymer is detected and warns', function(done) {
       var constants = require('../lib/constants');
       var input = path.resolve('test/html/old-polymer.html');
@@ -607,9 +630,11 @@ suite('Vulcan', function() {
         }
         var comments = dom5.nodeWalkAll(doc, dom5.isCommentNode);
         assert.equal(comments.length, 1);
+        var commentActual = dom5.getTextContent(comments[0]).trim();
+        assert.equal('@license test', commentActual);
         done();
       };
-      process(inputPath, callback, options);
+      process('test/html/comments.html', callback, options);
     });
 
     test('Comments are kept by default', function(done) {
@@ -621,9 +646,18 @@ suite('Vulcan', function() {
           return done(err);
         }
         var comments = dom5.nodeWalkAll(doc, dom5.isCommentNode);
-        assert.equal(comments.length, 2);
-        assert.equal(dom5.getTextContent(comments[0]).trim(), 'comment in import');
-        assert.equal(dom5.getTextContent(comments[1]).trim(), 'comment in main');
+        assert.equal(comments.length, 5);
+        var expectedComments = [
+          '@license test',
+          'comment in import',
+          '@license test',
+          'comment in import',
+          'comment in main'
+        ];
+        var actualComments = comments.map(function(c) {
+          return dom5.getTextContent(c).trim();
+        });
+        assert.deepEqual(expectedComments, actualComments);
         done();
       };
       process('test/html/comments.html', callback, options);
@@ -706,6 +740,21 @@ suite('Vulcan', function() {
       };
       process('test/html/xss.html', callback, options);
     });
+
+    test('Inlined Scripts are in the expected order', function(done) {
+      var callback = function(err, doc) {
+        if (err) {
+          return done(err);
+        }
+        var scripts = dom5.queryAll(doc, matchers.JS_INLINE);
+        var contents = scripts.map(function(script) {
+          return dom5.getTextContent(script);
+        });
+        assert.deepEqual(['"First"', '"Second"'], contents);
+        done();
+      };
+      process('test/html/reordered/in.html', callback, options);
+    });
   });
 
   suite('Inline CSS', function() {