fix(bundler): use topological sort to ensure shim order

closes #955 again
aurelia · Nov 16, 2018 · 6721519 · 6721519
1 parent 68e9de0
commit 6721519
Show file tree

Hide file tree

Showing 2 changed files with 126 additions and 32 deletions.
diff --git a/lib/build/bundle.js b/lib/build/bundle.js
@@ -132,7 +132,45 @@ exports.Bundle = class {
   }
 
   getBundledFiles() {
-    return uniqueBy(this.includes.reduce((a, b) => a.concat(b.getAllFiles()), []), (file) => file.path);
+    // Sort files by moduleId and deps to be sure they will always be
+    // concatenated in the same order, so revision hash won't change.
+    // https://github.com/aurelia/cli/issues/955#issuecomment-439253048
+    // Topological sort for shim packages.
+
+    let bundleFiles = uniqueBy(
+      this.includes.reduce((a, b) => a.concat(b.getAllFiles()), []),
+      file => file.path
+    ).sort((a, b) => {
+      // alphabetical sorting based on moduleId
+      if (a.moduleId > b.moduleId) return 1;
+      if (b.moduleId > a.moduleId) return -1;
+      return 0;
+    });
+
+    const sorted = [];
+    const visited = {};
+
+    const visit = file => {
+      const {moduleId, dependencyInclusion} = file;
+      if (visited[moduleId]) return;
+      visited[moduleId] = true;
+
+      if (dependencyInclusion) {
+        const {deps} = dependencyInclusion.description.loaderConfig;
+        if (deps) {
+          deps.forEach(packageName => {
+            bundleFiles.filter(f =>
+              f.dependencyInclusion && f.dependencyInclusion.description.name === packageName
+            ).forEach(visit);
+          });
+        }
+      }
+
+      sorted.push(file);
+    };
+
+    bundleFiles.forEach(visit);
+    return sorted;
   }
 
   write(platform) {
@@ -155,32 +193,9 @@ exports.Bundle = class {
       });
     }
 
-    // Sort files by moduleId and deps to be sure they will always be
-    // concatenated in the same order, so revision hash won't change.
-    let bundleFiles = this.getBundledFiles()
-      .sort((a, b) => {
-        // alphabetical sorting based on moduleId
-        if (a.moduleId > b.moduleId) return 1;
-        if (b.moduleId > a.moduleId) return -1;
-        return 0;
-      })
-      .sort((a, b) => {
-        // for shim with deps, make sure they are in proper order
-        if (a.dependencyInclusion && b.dependencyInclusion) {
-          let aPackageName = a.dependencyInclusion.description.name;
-          let bPackageName = b.dependencyInclusion.description.name;
-          let aDeps = a.dependencyInclusion.description.loaderConfig.deps;
-          let bDeps = b.dependencyInclusion.description.loaderConfig.deps;
-
-          // a must be after b
-          if (aDeps && aDeps.indexOf(bPackageName) !== -1) return 1;
-          // b must be after a
-          if (bDeps && bDeps.indexOf(aPackageName) !== -1) return -1;
-        }
-        return 0;
-      });
+    let bundleFiles = this.getBundledFiles();
 
-    // If bundle files like jquery does AMD define by itself: define('jquery', ...),
+    // If file like jquery does AMD define by itself: define('jquery', ...),
     // which bypass writeTransform lib/build/amodro-trace/write/defines,
     // alias created by dependency-inclusion on jquey main file:
     // define('jquery', ['jquery/dist/jquery'], ...) will be ignored.

diff --git a/spec/lib/build/bundle.spec.js b/spec/lib/build/bundle.spec.js
@@ -241,9 +241,9 @@ describe('the Bundle module', () => {
   });
 
   it('getBundledFiles returns all files of all includes', () => {
-    let aFile = {path: 'a.js'};
-    let bFile = {path: 'b.js'};
-    let cFile = {path: 'c.js'};
+    let aFile = {path: 'a.js', moduleId: 'a'};
+    let bFile = {path: 'b.js', moduleId: 'b'};
+    let cFile = {path: 'c.js', moduleId: 'c'};
 
     sut.includes = [{
       getAllFiles: () => [aFile, bFile]
@@ -255,9 +255,9 @@ describe('the Bundle module', () => {
   });
 
   it('getBundledFiles returns unique files of all includes', () => {
-    let aFile = {path: 'a.js'};
-    let bFile = {path: 'b.js'};
-    let cFile = {path: 'c.js'};
+    let aFile = {path: 'a.js', moduleId: 'a'};
+    let bFile = {path: 'b.js', moduleId: 'b'};
+    let cFile = {path: 'c.js', moduleId: 'c'};
 
     sut.includes = [{
       getAllFiles: () => [aFile, bFile]
@@ -268,6 +268,85 @@ describe('the Bundle module', () => {
     expect(sut.getBundledFiles()).toEqual([aFile, bFile, cFile]);
   });
 
+  it('getBundledFiles sorts shim', () => {
+    let aFile = {
+      path: 'node_modules/a/dist/index',
+      moduleId: 'a/dist/index',
+      dependencyInclusion: {
+        description: {
+          name: 'a',
+          loaderConfig: {
+            deps: ['c']
+          }
+        }
+      }
+    };
+    let bFile = {path: 'b.js', moduleId: 'b'};
+    let cFile = {
+      path: 'node_modules/c/dist/index',
+      moduleId: 'c/dist/index',
+      dependencyInclusion: {
+        description: {
+          name: 'c',
+          loaderConfig: {}
+        }
+      }
+    };
+
+    sut.includes = [{
+      getAllFiles: () => [aFile, bFile]
+    }, {
+      getAllFiles: () => [cFile]
+    }];
+
+    expect(sut.getBundledFiles()).toEqual([cFile, aFile, bFile]);
+  });
+
+  it('getBundledFiles sorts deep shim', () => {
+    let aFile = {
+      path: 'node_modules/a/dist/index',
+      moduleId: 'a/dist/index',
+      dependencyInclusion: {
+        description: {
+          name: 'a',
+          loaderConfig: {
+            deps: ['b']
+          }
+        }
+      }
+    };
+    let bFile = {
+      path: 'node_modules/b/dist/index',
+      moduleId: 'b/dist/index',
+      dependencyInclusion: {
+        description: {
+          name: 'b',
+          loaderConfig: {
+            deps: ['c']
+          }
+        }
+      }
+    };
+    let cFile = {
+      path: 'node_modules/c/dist/index',
+      moduleId: 'c/dist/index',
+      dependencyInclusion: {
+        description: {
+          name: 'c',
+          loaderConfig: {}
+        }
+      }
+    };
+
+    sut.includes = [{
+      getAllFiles: () => [aFile, bFile]
+    }, {
+      getAllFiles: () => [cFile]
+    }];
+
+    expect(sut.getBundledFiles()).toEqual([cFile, bFile, aFile]);
+  });
+
   it('configures dependencies in the same order as they were entered to prevent a wrong module load order', done => {
     let bundler = new BundlerMock();
     let configuredDependencies = [];