From 61b4334ff7cdca59bbadec583261d9b2adfb8da4 Mon Sep 17 00:00:00 2001 From: Gora Kong Date: Fri, 25 Feb 2022 17:05:21 -0800 Subject: [PATCH 1/7] working repro --- .../scope-hoisting/es6/unused-require/index.js | 3 +++ .../es6/unused-require/library/foo/index.js | 2 ++ .../es6/unused-require/library/foo/other.js | 1 + .../es6/unused-require/library/foo/package.json | 3 +++ .../es6/unused-require/library/index.js | 3 +++ .../core/integration-tests/test/scope-hoisting.js | 12 ++++++++++++ 6 files changed, 24 insertions(+) create mode 100644 packages/core/integration-tests/test/integration/scope-hoisting/es6/unused-require/index.js create mode 100644 packages/core/integration-tests/test/integration/scope-hoisting/es6/unused-require/library/foo/index.js create mode 100644 packages/core/integration-tests/test/integration/scope-hoisting/es6/unused-require/library/foo/other.js create mode 100644 packages/core/integration-tests/test/integration/scope-hoisting/es6/unused-require/library/foo/package.json create mode 100644 packages/core/integration-tests/test/integration/scope-hoisting/es6/unused-require/library/index.js diff --git a/packages/core/integration-tests/test/integration/scope-hoisting/es6/unused-require/index.js b/packages/core/integration-tests/test/integration/scope-hoisting/es6/unused-require/index.js new file mode 100644 index 00000000000..5ed2ae0a6db --- /dev/null +++ b/packages/core/integration-tests/test/integration/scope-hoisting/es6/unused-require/index.js @@ -0,0 +1,3 @@ +import {foo} from './library'; + +output = foo; diff --git a/packages/core/integration-tests/test/integration/scope-hoisting/es6/unused-require/library/foo/index.js b/packages/core/integration-tests/test/integration/scope-hoisting/es6/unused-require/library/foo/index.js new file mode 100644 index 00000000000..6f1f6971ff2 --- /dev/null +++ b/packages/core/integration-tests/test/integration/scope-hoisting/es6/unused-require/library/foo/index.js @@ -0,0 +1,2 @@ +export const unusedFoo = 'unusedFoo'; +export { foo } from './other'; diff --git a/packages/core/integration-tests/test/integration/scope-hoisting/es6/unused-require/library/foo/other.js b/packages/core/integration-tests/test/integration/scope-hoisting/es6/unused-require/library/foo/other.js new file mode 100644 index 00000000000..3329a7d972f --- /dev/null +++ b/packages/core/integration-tests/test/integration/scope-hoisting/es6/unused-require/library/foo/other.js @@ -0,0 +1 @@ +export const foo = 'foo'; diff --git a/packages/core/integration-tests/test/integration/scope-hoisting/es6/unused-require/library/foo/package.json b/packages/core/integration-tests/test/integration/scope-hoisting/es6/unused-require/library/foo/package.json new file mode 100644 index 00000000000..a43829151e1 --- /dev/null +++ b/packages/core/integration-tests/test/integration/scope-hoisting/es6/unused-require/library/foo/package.json @@ -0,0 +1,3 @@ +{ + "sideEffects": false +} diff --git a/packages/core/integration-tests/test/integration/scope-hoisting/es6/unused-require/library/index.js b/packages/core/integration-tests/test/integration/scope-hoisting/es6/unused-require/library/index.js new file mode 100644 index 00000000000..cc44b9b5e6e --- /dev/null +++ b/packages/core/integration-tests/test/integration/scope-hoisting/es6/unused-require/library/index.js @@ -0,0 +1,3 @@ +export { foo, unusedFoo } from './foo'; + +eval('') diff --git a/packages/core/integration-tests/test/scope-hoisting.js b/packages/core/integration-tests/test/scope-hoisting.js index b1632bfc83e..66c930435a4 100644 --- a/packages/core/integration-tests/test/scope-hoisting.js +++ b/packages/core/integration-tests/test/scope-hoisting.js @@ -5315,4 +5315,16 @@ describe('scope hoisting', function () { let output = await run(b); assert.strictEqual(output, 'bar foo bar'); }); + + it("not insert unused requires that aren't registered anywhere", async function () { + let b = await bundle( + path.join( + __dirname, + '/integration/scope-hoisting/es6/unused-require/index.js', + ), + ); + + let output = await run(b); + assert.strictEqual(output, 'foo'); + }); }); From 0f23e466eea77b627a4d60cbb0d1886b32568ce8 Mon Sep 17 00:00:00 2001 From: Gora Kong Date: Tue, 1 Mar 2022 14:46:32 -0800 Subject: [PATCH 2/7] fix test regressions --- .../packagers/js/src/ScopeHoistingPackager.js | 18 +++++++++++++++--- 1 file changed, 15 insertions(+), 3 deletions(-) diff --git a/packages/packagers/js/src/ScopeHoistingPackager.js b/packages/packagers/js/src/ScopeHoistingPackager.js index 87ed3952bd1..1501704b141 100644 --- a/packages/packagers/js/src/ScopeHoistingPackager.js +++ b/packages/packagers/js/src/ScopeHoistingPackager.js @@ -741,10 +741,22 @@ ${code} this.hoistedRequires.set(dep.id, hoisted); } - hoisted.set( - resolvedAsset.id, - `var $${publicId} = parcelRequire(${JSON.stringify(publicId)});`, + let resolvedAssetUsedSymbols = nullthrows( + this.bundleGraph.getUsedSymbols(resolvedAsset), ); + + if ( + nullthrows(this.bundleGraph.getUsedSymbols(dep)).has(exportSymbol) || + resolvedAssetUsedSymbols.has(exportSymbol) || + (exportSymbol === 'default' && + resolvedAssetUsedSymbols.has(exportSymbol)) || + (exportSymbol === '*' && resolvedAssetUsedSymbols.size > 0) + ) { + hoisted.set( + resolvedAsset.id, + `var $${publicId} = parcelRequire(${JSON.stringify(publicId)});`, + ); + } } if (isWrapped) { From f6b08cfe84e9726db646c72ed30fbcea6521e13b Mon Sep 17 00:00:00 2001 From: Gora Kong Date: Wed, 2 Mar 2022 13:26:18 -0800 Subject: [PATCH 3/7] add comment --- packages/packagers/js/src/ScopeHoistingPackager.js | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/packages/packagers/js/src/ScopeHoistingPackager.js b/packages/packagers/js/src/ScopeHoistingPackager.js index 9537e915f1a..8dbdb71ab53 100644 --- a/packages/packagers/js/src/ScopeHoistingPackager.js +++ b/packages/packagers/js/src/ScopeHoistingPackager.js @@ -732,7 +732,7 @@ ${code} let staticExports = resolvedAsset.meta.staticExports !== false; let publicId = this.bundleGraph.getAssetPublicId(resolvedAsset); - // If the rsolved asset is wrapped, but imported at the top-level by this asset, + // If the resolved asset is wrapped, but imported at the top-level by this asset, // then we hoist parcelRequire calls to the top of this asset so side effects run immediately. if (isWrapped && dep && !dep?.meta.shouldWrap && symbol !== false) { let hoisted = this.hoistedRequires.get(dep.id); @@ -745,13 +745,19 @@ ${code} this.bundleGraph.getUsedSymbols(resolvedAsset), ); - if ( + // Check to see if the exportSymbol is in the resolved asset or the dependency's + // set of used symbols. If the exportSymbol is 'default', check that the + // default export from the resolved asset is used. + // If the exportSymbol is '*', a non-empty set of the resolved asset's used symbols + // indicates the export(s) is used. + let isExportSymbolUsed = resolvedAssetUsedSymbols.has(exportSymbol) || nullthrows(this.bundleGraph.getUsedSymbols(dep)).has(exportSymbol) || (exportSymbol === 'default' && resolvedAssetUsedSymbols.has(exportSymbol)) || - (exportSymbol === '*' && resolvedAssetUsedSymbols.size > 0) - ) { + (exportSymbol === '*' && resolvedAssetUsedSymbols.size > 0); + + if (isExportSymbolUsed) { hoisted.set( resolvedAsset.id, `var $${publicId} = parcelRequire(${JSON.stringify(publicId)});`, From e092f8563687760cf979095de362e02c5ff95432 Mon Sep 17 00:00:00 2001 From: Gora Kong Date: Tue, 15 Mar 2022 15:23:15 -0700 Subject: [PATCH 4/7] add extra condition + comments --- packages/packagers/js/src/ScopeHoistingPackager.js | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/packages/packagers/js/src/ScopeHoistingPackager.js b/packages/packagers/js/src/ScopeHoistingPackager.js index 8dbdb71ab53..9cbd79b96c1 100644 --- a/packages/packagers/js/src/ScopeHoistingPackager.js +++ b/packages/packagers/js/src/ScopeHoistingPackager.js @@ -745,14 +745,21 @@ ${code} this.bundleGraph.getUsedSymbols(resolvedAsset), ); - // Check to see if the exportSymbol is in the resolved asset or the dependency's - // set of used symbols. If the exportSymbol is 'default', check that the + // Check to see if the exportSymbol is in the resolved asset + // or the dependency's set of used symbols, + // or if any of the resolved asset's dependencies export the symbol. + // If the exportSymbol is 'default', check that the // default export from the resolved asset is used. // If the exportSymbol is '*', a non-empty set of the resolved asset's used symbols // indicates the export(s) is used. let isExportSymbolUsed = resolvedAssetUsedSymbols.has(exportSymbol) || nullthrows(this.bundleGraph.getUsedSymbols(dep)).has(exportSymbol) || + this.bundleGraph + .getDependencies(resolvedAsset) + .some(dep => + nullthrows(this.bundleGraph.getUsedSymbols(dep)).has(exportSymbol), + ) || (exportSymbol === 'default' && resolvedAssetUsedSymbols.has(exportSymbol)) || (exportSymbol === '*' && resolvedAssetUsedSymbols.size > 0); From d13f685d3f081f594385c062cf02201539b67800 Mon Sep 17 00:00:00 2001 From: Gora Kong Date: Wed, 23 Mar 2022 12:25:11 -0700 Subject: [PATCH 5/7] remove redundant condition --- packages/packagers/js/src/ScopeHoistingPackager.js | 2 -- 1 file changed, 2 deletions(-) diff --git a/packages/packagers/js/src/ScopeHoistingPackager.js b/packages/packagers/js/src/ScopeHoistingPackager.js index 9cbd79b96c1..35490b62ea3 100644 --- a/packages/packagers/js/src/ScopeHoistingPackager.js +++ b/packages/packagers/js/src/ScopeHoistingPackager.js @@ -760,8 +760,6 @@ ${code} .some(dep => nullthrows(this.bundleGraph.getUsedSymbols(dep)).has(exportSymbol), ) || - (exportSymbol === 'default' && - resolvedAssetUsedSymbols.has(exportSymbol)) || (exportSymbol === '*' && resolvedAssetUsedSymbols.size > 0); if (isExportSymbolUsed) { From 74a7da4626ff1ec7bd577ab2b8c262e3a445a659 Mon Sep 17 00:00:00 2001 From: Gora Kong Date: Wed, 23 Mar 2022 17:04:28 -0700 Subject: [PATCH 6/7] clean up comment --- packages/packagers/js/src/ScopeHoistingPackager.js | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/packages/packagers/js/src/ScopeHoistingPackager.js b/packages/packagers/js/src/ScopeHoistingPackager.js index 35490b62ea3..88c9bda1e0b 100644 --- a/packages/packagers/js/src/ScopeHoistingPackager.js +++ b/packages/packagers/js/src/ScopeHoistingPackager.js @@ -745,11 +745,8 @@ ${code} this.bundleGraph.getUsedSymbols(resolvedAsset), ); - // Check to see if the exportSymbol is in the resolved asset - // or the dependency's set of used symbols, - // or if any of the resolved asset's dependencies export the symbol. - // If the exportSymbol is 'default', check that the - // default export from the resolved asset is used. + // Check to see if the exportSymbol is in the resolved asset or the dependency's + // set of used symbols, or if any of the resolved asset's dependencies export the symbol. // If the exportSymbol is '*', a non-empty set of the resolved asset's used symbols // indicates the export(s) is used. let isExportSymbolUsed = From c0988bbad75b647faa7accebd6000e2efb024488 Mon Sep 17 00:00:00 2001 From: Gora Kong Date: Thu, 14 Apr 2022 15:45:08 -0700 Subject: [PATCH 7/7] check instead for when no parcelRequire.register was generated --- .../packagers/js/src/ScopeHoistingPackager.js | 37 +++++++------------ 1 file changed, 13 insertions(+), 24 deletions(-) diff --git a/packages/packagers/js/src/ScopeHoistingPackager.js b/packages/packagers/js/src/ScopeHoistingPackager.js index 2d7e9d33bc7..ce69c263ff2 100644 --- a/packages/packagers/js/src/ScopeHoistingPackager.js +++ b/packages/packagers/js/src/ScopeHoistingPackager.js @@ -762,37 +762,26 @@ ${code} // If the resolved asset is wrapped, but imported at the top-level by this asset, // then we hoist parcelRequire calls to the top of this asset so side effects run immediately. - if (isWrapped && dep && !dep?.meta.shouldWrap && symbol !== false) { + if ( + isWrapped && + dep && + !dep?.meta.shouldWrap && + symbol !== false && + // Only do this if the asset is part of a different bundle (so it was definitely + // parcelRequire.register'ed there), or if it is indeed registered in this bundle. + (!this.bundle.hasAsset(resolvedAsset) || + !this.shouldSkipAsset(resolvedAsset)) + ) { let hoisted = this.hoistedRequires.get(dep.id); if (!hoisted) { hoisted = new Map(); this.hoistedRequires.set(dep.id, hoisted); } - let resolvedAssetUsedSymbols = nullthrows( - this.bundleGraph.getUsedSymbols(resolvedAsset), + hoisted.set( + resolvedAsset.id, + `var $${publicId} = parcelRequire(${JSON.stringify(publicId)});`, ); - - // Check to see if the exportSymbol is in the resolved asset or the dependency's - // set of used symbols, or if any of the resolved asset's dependencies export the symbol. - // If the exportSymbol is '*', a non-empty set of the resolved asset's used symbols - // indicates the export(s) is used. - let isExportSymbolUsed = - resolvedAssetUsedSymbols.has(exportSymbol) || - nullthrows(this.bundleGraph.getUsedSymbols(dep)).has(exportSymbol) || - this.bundleGraph - .getDependencies(resolvedAsset) - .some(dep => - nullthrows(this.bundleGraph.getUsedSymbols(dep)).has(exportSymbol), - ) || - (exportSymbol === '*' && resolvedAssetUsedSymbols.size > 0); - - if (isExportSymbolUsed) { - hoisted.set( - resolvedAsset.id, - `var $${publicId} = parcelRequire(${JSON.stringify(publicId)});`, - ); - } } if (isWrapped) {