fixes #933 : JSPackager deduplication now accounts for differences in…

… absolute dependency paths (#1011)

src/packagers/JSPackager.js

@@ -3,6 +3,7 @@ const path = require('path');
 const Packager = require('./Packager');
 const urlJoin = require('../utils/urlJoin');
 const lineCounter = require('../utils/lineCounter');
+const objectHash = require('../utils/objectHash');
 
 const prelude = {
   source: fs
@@ -35,13 +36,14 @@ class JSPackager extends Packager {
   }
 
   async addAsset(asset) {
-    if (this.dedupe.has(asset.generated.js)) {
+    let key = this.dedupeKey(asset);
+    if (this.dedupe.has(key)) {
       return;
     }
 
     // Don't dedupe when HMR is turned on since it messes with the asset ids
     if (!this.options.hmr) {
-      this.dedupe.set(asset.generated.js, asset.id);
+      this.dedupe.set(key, asset.id);
     }
 
     let deps = {};
@@ -60,7 +62,7 @@ class JSPackager extends Packager {
         deps[dep.name] = bundles;
         this.bundleLoaders.add(mod.type);
       } else {
-        deps[dep.name] = this.dedupe.get(mod.generated.js) || mod.id;
+        deps[dep.name] = this.dedupe.get(this.dedupeKey(mod)) || mod.id;
 
         // If the dep isn't in this bundle, add it to the list of external modules to preload.
         // Only do this if this is the root JS bundle, otherwise they will have already been
@@ -93,6 +95,14 @@ class JSPackager extends Packager {
     return name;
   }
 
+  dedupeKey(asset) {
+    // cannot rely *only* on generated JS for deduplication because paths like
+    // `../` can cause 2 identical JS files to behave differently depending on
+    // where they are located on the filesystem
+    let deps = Array.from(asset.depAssets.values(), dep => dep.name).sort();
+    return objectHash([asset.generated.js, deps]);
+  }
+
   async writeModule(id, code, deps = {}, map) {
     let wrapped = this.first ? '' : ',';
     wrapped +=

test/integration/js-different-contents/hello.js

@@ -0,0 +1,2 @@
+const value = 'Hello'
+export default value

test/integration/js-different-contents/index.js

@@ -0,0 +1,4 @@
+import hello from './hello.js'
+import world from './world.js'
+
+export default `${hello} ${world}!`

test/integration/js-different-contents/world.js

@@ -0,0 +1,2 @@
+const value = 'World'
+export default value

test/integration/js-same-contents-different-dependencies/hello/index.js

@@ -0,0 +1,3 @@
+// hello/index.js and world/index.js are exactly the same content-wise, but in different locations
+import value from './value'
+export default value

test/integration/js-same-contents-different-dependencies/hello/value.js

@@ -0,0 +1,2 @@
+const value = 'Hello'
+export default value

test/integration/js-same-contents-different-dependencies/index.js

@@ -0,0 +1,4 @@
+import hello from './hello'
+import world from './world'
+
+export default `${hello} ${world}!`

test/integration/js-same-contents-different-dependencies/world/index.js

@@ -0,0 +1,3 @@
+// hello/index.js and world/index.js are exactly the same content-wise, but in different locations
+import value from './value'
+export default value

test/integration/js-same-contents-different-dependencies/world/value.js

@@ -0,0 +1,2 @@
+const value = 'World'
+export default value

test/integration/js-same-contents-same-dependencies/hello1.js

@@ -0,0 +1,2 @@
+const value = 'Hello'
+export default value

test/integration/js-same-contents-same-dependencies/hello2.js

@@ -0,0 +1,2 @@
+const value = 'Hello'
+export default value

test/integration/js-same-contents-same-dependencies/index.js

@@ -0,0 +1,4 @@
+import hello1 from './hello1'
+import hello2 from './hello2'
+
+export default `${hello1} ${hello2}!`

test/javascript.js

@@ -968,4 +968,55 @@ describe('javascript', function() {
     await run(b, {define: mockDefine, module: undefined});
     assert.equal(test, 2);
   });
+
+  it('should not dedupe imports with different contents', async function() {
+    let b = await bundle(
+      __dirname + `/integration/js-different-contents/index.js`,
+      {
+        hmr: false // enable asset dedupe in JSPackager
+      }
+    );
+
+    let module = await run(b);
+    assert.equal(module.default, 'Hello World!');
+  });
+
+  it('should not dedupe imports with same content but different absolute dependency paths', async function() {
+    let b = await bundle(
+      __dirname +
+        `/integration/js-same-contents-different-dependencies/index.js`,
+      {
+        hmr: false // enable asset dedupe in JSPackager
+      }
+    );
+
+    let module = await run(b);
+    assert.equal(module.default, 'Hello World!');
+  });
+
+  it('should dedupe imports with same content and same dependency paths', async function() {
+    let b = await bundle(
+      __dirname + `/integration/js-same-contents-same-dependencies/index.js`,
+      {
+        hmr: false // enable asset dedupe in JSPackager
+      }
+    );
+    const {rootDir} = b.entryAsset.options;
+    const dedupedAssets = Array.from(b.offsets.keys()).map(asset => asset.name);
+    assert.equal(dedupedAssets.length, 2);
+    assert(dedupedAssets.includes(path.join(rootDir, 'index.js')));
+    assert(
+      dedupedAssets.includes(path.join(rootDir, 'hello1.js')) ||
+        dedupedAssets.includes(path.join(rootDir, 'hello2.js'))
+    );
+    assert(
+      !(
+        dedupedAssets.includes(path.join(rootDir, 'hello1.js')) &&
+        dedupedAssets.includes(path.join(rootDir, 'hello2.js'))
+      )
+    );
+
+    let module = await run(b);
+    assert.equal(module.default, 'Hello Hello!');
+  });
 });