Repository: Polymer/vulcanize (js)

Issue:
Vulcanize rearranges script tags which breaks things
Files for repro: https://dl.dropboxusercontent.com/u/15217362/vulcanizebug.zip

If you vulcanize an import that looks like this:

```
<script src="before.js"></script>
<link rel="import" href="nested.html" />
<script src="after.js"></script>
```

You get output that looks like this:

```
<script src="after.js"></script>
<script src="before.js"></script>
```

Obviously since after.js depends on things defined in before.js, it breaks. So vulcanize ends up mangling the order of scripts and breaks things.

Pull request:
Better preserve script order when interleaving script and imports
Fixes #325

Patch:
diff --git a/lib/vulcan.js b/lib/vulcan.js
index 307b953..304c1e3 100644
--- a/lib/vulcan.js
+++ b/lib/vulcan.js
@@ -190,7 +190,7 @@ Vulcan.prototype = {
     return false;
   },
 
-  flatten: function flatten(tree, bodyFragment, mainDocUrl) {
+  flatten: function flatten(tree, bodyFragment, mainDocUrl, isMainDocument) {
     var doc = tree.html.ast;
     var imports = tree.imports;
     var head = dom5.query(doc, matchers.head);
@@ -204,7 +204,8 @@ Vulcan.prototype = {
     this.pathResolver.acid(doc, tree.href);
     if (imports) {
       // flatten all of <head> into one list
-      var flatHead = dom5.queryAll(head, function(){ return true; });
+      var flatHead = dom5.nodeWalkAll(head, function(){ return true; });
+      var currentBodyFragment = dom5.constructors.fragment();
       for (var i = 0, im, thisImport; i < imports.length; i++) {
         im = imports[i];
         thisImport = importNodes[i];
@@ -219,7 +220,7 @@ Vulcan.prototype = {
         if (this.isExcludedImport(im)) {
           continue;
         }
-        var importDoc = this.flatten(im, bodyFragment, mainDocUrl);
+        var importDoc = this.flatten(im, currentBodyFragment, mainDocUrl);
         // rewrite urls
         this.pathResolver.resolvePaths(importDoc, im.href, tree.href);
         var importHead = dom5.query(importDoc, matchers.head);
@@ -235,20 +236,16 @@ Vulcan.prototype = {
         licenseComments = licenseComments.concat(importHtml.childNodes.filter(this.isLicenseComment));
         // put license comments at the top of <head>
         importHeadChildren = licenseComments.concat(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);
+          importHeadChildren.forEach(this.reparent(currentBodyFragment));
+          currentBodyFragment.childNodes = currentBodyFragment.childNodes.concat(importHeadChildren);
         }
         // defer body children to be inlined in-order
         if (importBodyChildren.length) {
           // adjust body urls to main document
-          this.pathResolver.resolvePaths(importBody, tree.href, mainDocUrl);
-          importBodyChildren.forEach(this.reparent(bodyFragment));
-          bodyFragment.childNodes = bodyFragment.childNodes.concat(importBodyChildren);
+          importBodyChildren.forEach(this.reparent(currentBodyFragment));
+          currentBodyFragment.childNodes = currentBodyFragment.childNodes.concat(importBodyChildren);
         }
         // to preserve script execution order, defer scripts after this import
         // into body
@@ -258,13 +255,24 @@ Vulcan.prototype = {
           var start = flatHead.indexOf(thisImport) + 1;
           var end = flatHead.indexOf(importNodes[i + 1]);
           var container = this.findElementsToMoveToBody(flatHead, start, end);
-          // repath nodes moving to bodyFragment
           if (container.childNodes.length) {
-            this.pathResolver.resolvePaths(container, tree.href, mainDocUrl);
-            dom5.append(bodyFragment, container);
+            dom5.append(currentBodyFragment, container);
           }
         }
+        // delete link
+        this.removeImportAndNewline(thisImport);
       }
+      // resolve paths
+      this.pathResolver.resolvePaths(currentBodyFragment, tree.href, mainDocUrl);
+      if (!isMainDocument && currentBodyFragment.childNodes.length) {
+        // prepend this import document's <head> into body fragment
+        // preserves order when being flattened into a parent import
+        var hn = head.childNodes;
+        hn.forEach(this.reparent(currentBodyFragment));
+        currentBodyFragment.childNodes = hn.concat(currentBodyFragment.childNodes);
+        head.childNodes = [];
+      }
+      dom5.append(bodyFragment, currentBodyFragment);
     }
     return doc;
   },
@@ -295,7 +303,7 @@ Vulcan.prototype = {
       dom5.append(parent, node);
     }
   },
-  
+
   fixFakeExternalScripts: function fixFakeExternalScripts(doc) {
     var scripts = dom5.queryAll(doc, matchers.JS_INLINE);
     scripts.map(function(script) {
@@ -304,7 +312,7 @@ Vulcan.prototype = {
       }
     });
   },
-  
+
   // inline scripts into document, returns a promise resolving to document.
   inlineScripts: function inlineScripts(doc, href) {
     var scripts = dom5.queryAll(doc, matchers.JS_SRC);
@@ -432,7 +440,7 @@ Vulcan.prototype = {
       var bodyFragment = dom5.constructors.element('div');
       dom5.setAttribute(bodyFragment, 'hidden', '');
       dom5.setAttribute(bodyFragment, 'by-vulcanize', '');
-      var flatDoc = this.flatten(tree, bodyFragment, tree.href);
+      var flatDoc = this.flatten(tree, bodyFragment, tree.href, true);
       var body = dom5.query(flatDoc, matchers.body);
       if (bodyFragment.childNodes.length) {
         this.prepend(body, bodyFragment);


Test patch:
diff --git a/test/html/imports/comment-in-import.html b/test/html/imports/comment-in-import.html
index 026cba5..1e0344d 100644
--- a/test/html/imports/comment-in-import.html
+++ b/test/html/imports/comment-in-import.html
@@ -1,7 +1,7 @@
-<!-- @license import -->
+<!-- @license import 1 -->
 <link rel="import" href="sub/second.html">
-<!-- comment in import -->
+<!-- comment in import 1 -->
 <div>
-  <!-- @license import -->
-  <!-- comment in import -->
+  <!-- @license import 2 -->
+  <!-- comment in import 2 -->
 </div>
diff --git a/test/html/scriptorder/after.js b/test/html/scriptorder/after.js
new file mode 100644
index 0000000..8bfa13f
--- /dev/null
+++ b/test/html/scriptorder/after.js
@@ -0,0 +1,2 @@
+
+console.log("Hello from after.js! BeforeJs.value = " + BeforeJs.value);
diff --git a/test/html/scriptorder/before.js b/test/html/scriptorder/before.js
new file mode 100644
index 0000000..7c7f439
--- /dev/null
+++ b/test/html/scriptorder/before.js
@@ -0,0 +1,4 @@
+
+console.log("Hello from before.js!");
+
+window.BeforeJs = { value: 77 };
\ No newline at end of file
diff --git a/test/html/scriptorder/cmd.txt b/test/html/scriptorder/cmd.txt
new file mode 100644
index 0000000..e4eceb0
--- /dev/null
+++ b/test/html/scriptorder/cmd.txt
@@ -0,0 +1,1 @@
+vulcanize --out-html index.min.html index.html
\ No newline at end of file
diff --git a/test/html/scriptorder/index.html b/test/html/scriptorder/index.html
new file mode 100644
index 0000000..fb9e989
--- /dev/null
+++ b/test/html/scriptorder/index.html
@@ -0,0 +1,9 @@
+<!DOCTYPE html>
+<html>
+<head>
+	<title>Vulcanize test</title>
+	<link rel="import" href="main.html" />
+</head>
+<body>
+</body>
+</html>
diff --git a/test/html/scriptorder/index.min.html b/test/html/scriptorder/index.min.html
new file mode 100644
index 0000000..3591cda
--- /dev/null
+++ b/test/html/scriptorder/index.min.html
@@ -0,0 +1,15 @@
+<!DOCTYPE html><html><head><meta charset="UTF-8">
+	<title>Vulcanize test</title>
+	</head>
+<body><div hidden="" by-vulcanize=""><script src="before.js">
+console.log("Hello from before.js!");
+
+window.BeforeJs = { value: 77 };</script><script src="after.js">
+console.log("Hello from after.js! BeforeJs.value = " + BeforeJs.value);
+</script>
+
+
+</div>
+
+
+</body></html>
diff --git a/test/html/scriptorder/main.html b/test/html/scriptorder/main.html
new file mode 100644
index 0000000..4592a6f
--- /dev/null
+++ b/test/html/scriptorder/main.html
@@ -0,0 +1,6 @@
+
+<script src="before.js"></script>
+
+<link rel="import" href="nested.html" />
+
+<script src="after.js"></script>
diff --git a/test/html/scriptorder/nested.html b/test/html/scriptorder/nested.html
new file mode 100644
index 0000000..b3fe68c
--- /dev/null
+++ b/test/html/scriptorder/nested.html
@@ -0,0 +1,2 @@
+
+<!-- contents not important -->
diff --git a/test/test.js b/test/test.js
index cdb62c4..b6f33d6 100644
--- a/test/test.js
+++ b/test/test.js
@@ -457,6 +457,22 @@ suite('Vulcan', function() {
       process('test/html/order-test.html', callback);
     });
 
+    test('exhaustive script order testing', function(done) {
+      process('test/html/scriptorder/index.html', function(err, doc) {
+        if (err) {
+          return done(err);
+        }
+        assert(doc);
+        var serialized = dom5.serialize(doc);
+        var beforeLoc = serialized.indexOf("window.BeforeJs");
+        var afterLoc = serialized.indexOf("BeforeJs.value");
+        assert.isBelow(beforeLoc, afterLoc);
+        done();
+      }, {
+        inlineScripts: true
+      });
+    });
+
     test('Old Polymer is detected and warns', function(done) {
       var constants = require('../lib/constants');
       var input = path.resolve('test/html/old-polymer.html');
@@ -696,9 +712,10 @@ suite('Vulcan', function() {
           return done(err);
         }
         var comments = dom5.nodeWalkAll(doc, dom5.isCommentNode);
-        assert.equal(comments.length, 2);
+        assert.equal(comments.length, 3);
         var commentsExpected = [
-          '@license import',
+          '@license import 2',
+          '@license import 1',
           '@license main'
         ];
         var commentsActual = comments.map(function(c) {
@@ -721,10 +738,10 @@ suite('Vulcan', function() {
         var comments = dom5.nodeWalkAll(doc, dom5.isCommentNode);
         var expectedComments = [
           '@license main',
-          '@license import',
-          'comment in import',
-          '@license import',
-          'comment in import',
+          '@license import 1',
+          'comment in import 1',
+          '@license import 2',
+          'comment in import 2',
           'comment in main'
         ];
         var actualComments = comments.map(function(c) {
@@ -959,6 +976,7 @@ suite('Vulcan', function() {
     });
   });
 
+
   suite('Input URL', function() {
     var options = {
       inputUrl: 'test/html/default.html'