From 869e5b32deac561e8e03a7ce67558dd875798bad Mon Sep 17 00:00:00 2001 From: Devon Govett Date: Thu, 11 Oct 2018 15:44:55 +0200 Subject: [PATCH 1/2] Don't dedupe assets that exist in more than one bundle --- src/packagers/JSPackager.js | 23 +++++++++----- test/integration/js-dedup-hoist/a.js | 3 ++ test/integration/js-dedup-hoist/hello1.js | 2 ++ test/integration/js-dedup-hoist/hello2.js | 2 ++ test/integration/js-dedup-hoist/index.js | 7 +++++ test/integration/js-dedup-hoist/package.json | 3 ++ test/javascript.js | 32 +++++++++++++++----- 7 files changed, 58 insertions(+), 14 deletions(-) create mode 100644 test/integration/js-dedup-hoist/a.js create mode 100644 test/integration/js-dedup-hoist/hello1.js create mode 100644 test/integration/js-dedup-hoist/hello2.js create mode 100644 test/integration/js-dedup-hoist/index.js create mode 100644 test/integration/js-dedup-hoist/package.json diff --git a/src/packagers/JSPackager.js b/src/packagers/JSPackager.js index 6464c7a25b8..028e3bb68aa 100644 --- a/src/packagers/JSPackager.js +++ b/src/packagers/JSPackager.js @@ -31,14 +31,23 @@ class JSPackager extends Packager { } async addAsset(asset) { - let key = this.dedupeKey(asset); - if (this.dedupe.has(key)) { - return; - } + // If this module is referenced by another JS bundle, it needs to be exposed externally. + // In that case, don't dedupe the asset as it would affect the module ids that are referenced by other bundles. + let isExposed = !Array.from(asset.parentDeps).every(dep => { + let depAsset = this.bundler.loadedAssets.get(dep.parent); + return this.bundle.assets.has(depAsset) || depAsset.type !== 'js'; + }); + + if (!isExposed) { + 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(key, asset.id); + // Don't dedupe when HMR is turned on since it messes with the asset ids + if (!this.options.hmr) { + this.dedupe.set(key, asset.id); + } } let deps = {}; diff --git a/test/integration/js-dedup-hoist/a.js b/test/integration/js-dedup-hoist/a.js new file mode 100644 index 00000000000..addf011baf7 --- /dev/null +++ b/test/integration/js-dedup-hoist/a.js @@ -0,0 +1,3 @@ +import hello2 from './hello2' + +export default `${hello2}` diff --git a/test/integration/js-dedup-hoist/hello1.js b/test/integration/js-dedup-hoist/hello1.js new file mode 100644 index 00000000000..1fdb8c5437e --- /dev/null +++ b/test/integration/js-dedup-hoist/hello1.js @@ -0,0 +1,2 @@ +const value = 'Hello' +export default value diff --git a/test/integration/js-dedup-hoist/hello2.js b/test/integration/js-dedup-hoist/hello2.js new file mode 100644 index 00000000000..1fdb8c5437e --- /dev/null +++ b/test/integration/js-dedup-hoist/hello2.js @@ -0,0 +1,2 @@ +const value = 'Hello' +export default value diff --git a/test/integration/js-dedup-hoist/index.js b/test/integration/js-dedup-hoist/index.js new file mode 100644 index 00000000000..f39a4a5d189 --- /dev/null +++ b/test/integration/js-dedup-hoist/index.js @@ -0,0 +1,7 @@ +import hello1 from './hello1' +import hello2 from './hello2' + +export default async function () { + let a = await import('./a'); + return `${hello1} ${hello2}! ${a.default}`; +} diff --git a/test/integration/js-dedup-hoist/package.json b/test/integration/js-dedup-hoist/package.json new file mode 100644 index 00000000000..3c1065f3deb --- /dev/null +++ b/test/integration/js-dedup-hoist/package.json @@ -0,0 +1,3 @@ +{ + "browserslist": ["last 1 Chrome version"] +} diff --git a/test/javascript.js b/test/javascript.js index 4f34102ca68..d620fc634f6 100644 --- a/test/javascript.js +++ b/test/javascript.js @@ -1520,17 +1520,17 @@ describe('javascript', function() { } ); 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'))); + const writtenAssets = Array.from(b.offsets.keys()).map(asset => asset.name); + assert.equal(writtenAssets.length, 2); + assert(writtenAssets.includes(path.join(rootDir, 'index.js'))); assert( - dedupedAssets.includes(path.join(rootDir, 'hello1.js')) || - dedupedAssets.includes(path.join(rootDir, 'hello2.js')) + writtenAssets.includes(path.join(rootDir, 'hello1.js')) || + writtenAssets.includes(path.join(rootDir, 'hello2.js')) ); assert( !( - dedupedAssets.includes(path.join(rootDir, 'hello1.js')) && - dedupedAssets.includes(path.join(rootDir, 'hello2.js')) + writtenAssets.includes(path.join(rootDir, 'hello1.js')) && + writtenAssets.includes(path.join(rootDir, 'hello2.js')) ) ); @@ -1538,6 +1538,24 @@ describe('javascript', function() { assert.equal(module.default, 'Hello Hello!'); }); + it('should not dedupe assets that exist in more than one bundle', async function() { + let b = await bundle( + path.join(__dirname, `/integration/js-dedup-hoist/index.js`), + { + hmr: false // enable asset dedupe in JSPackager + } + ); + const {rootDir} = b.entryAsset.options; + const writtenAssets = Array.from(b.offsets.keys()).map(asset => asset.name); + assert( + writtenAssets.includes(path.join(rootDir, 'hello1.js')) && + writtenAssets.includes(path.join(rootDir, 'hello2.js')) + ); + + let module = await run(b); + assert.equal(await module.default(), 'Hello Hello! Hello'); + }); + it('should support importing HTML from JS async', async function() { let b = await bundle( path.join(__dirname, '/integration/import-html-async/index.js'), From 5b5c0c3b273bd1b766eeeea20905ed0d293033f2 Mon Sep 17 00:00:00 2001 From: Devon Govett Date: Thu, 11 Oct 2018 18:17:41 +0200 Subject: [PATCH 2/2] Fix node 6 test --- test/integration/js-dedup-hoist/index.js | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/test/integration/js-dedup-hoist/index.js b/test/integration/js-dedup-hoist/index.js index f39a4a5d189..95a5b7774f2 100644 --- a/test/integration/js-dedup-hoist/index.js +++ b/test/integration/js-dedup-hoist/index.js @@ -1,7 +1,8 @@ import hello1 from './hello1' import hello2 from './hello2' -export default async function () { - let a = await import('./a'); - return `${hello1} ${hello2}! ${a.default}`; +export default function () { + return import('./a').then(function (a) { + return `${hello1} ${hello2}! ${a.default}`; + }); }