From 2b0b677e63b182f6d06c5e48b8d4626aea9b9e06 Mon Sep 17 00:00:00 2001 From: Will Binns-Smith Date: Mon, 11 Apr 2022 15:50:57 -0700 Subject: [PATCH] ScopeHoistingPackager: Wrap assets when any ancestor is wrapped (#7883) * Add test for wrapping assets, even if the first ancestor is not * ScopeHoistingPackager: Handle different wrapped ancestries in wrapping dfs --- packages/core/core/src/BundleGraph.js | 9 ++++++-- packages/core/core/src/public/Bundle.js | 6 +++++- .../es6/multiple-ancestors-wrap/index.js | 4 ++++ .../shouldBeWrapped.js | 1 + .../es6/multiple-ancestors-wrap/wraps.js | 5 +++++ .../integration-tests/test/scope-hoisting.js | 21 +++++++++++++++++++ packages/core/types/index.js | 5 ++++- .../packagers/js/src/ScopeHoistingPackager.js | 20 +++++++++++++++--- 8 files changed, 64 insertions(+), 7 deletions(-) create mode 100644 packages/core/integration-tests/test/integration/scope-hoisting/es6/multiple-ancestors-wrap/index.js create mode 100644 packages/core/integration-tests/test/integration/scope-hoisting/es6/multiple-ancestors-wrap/shouldBeWrapped.js create mode 100644 packages/core/integration-tests/test/integration/scope-hoisting/es6/multiple-ancestors-wrap/wraps.js diff --git a/packages/core/core/src/BundleGraph.js b/packages/core/core/src/BundleGraph.js index 537ecc0f78a..3f0de396c84 100644 --- a/packages/core/core/src/BundleGraph.js +++ b/packages/core/core/src/BundleGraph.js @@ -860,10 +860,12 @@ export default class BundleGraph { traverseAssets( bundle: Bundle, visit: GraphVisitor, + startAsset?: Asset, ): ?TContext { return this.traverseBundle( bundle, mapVisitor(node => (node.type === 'asset' ? node.value : null), visit), + startAsset, ); } @@ -1057,8 +1059,9 @@ export default class BundleGraph { traverseBundle( bundle: Bundle, visit: GraphVisitor, + startAsset?: Asset, ): ?TContext { - let entries = true; + let entries = !startAsset; let bundleNodeId = this._graph.getNodeIdByContentKey(bundle.id); // A modified DFS traversal which traverses entry assets in the same order @@ -1085,7 +1088,9 @@ export default class BundleGraph { actions.skipChildren(); }, visit), - startNodeId: bundleNodeId, + startNodeId: startAsset + ? this._graph.getNodeIdByContentKey(startAsset.id) + : bundleNodeId, getChildren: nodeId => { let children = this._graph .getNodeIdsConnectedFrom(nodeId) diff --git a/packages/core/core/src/public/Bundle.js b/packages/core/core/src/public/Bundle.js index 3581678d33d..fb95d1a5359 100644 --- a/packages/core/core/src/public/Bundle.js +++ b/packages/core/core/src/public/Bundle.js @@ -186,10 +186,14 @@ export class Bundle implements IBundle { ); } - traverseAssets(visit: GraphVisitor): ?TContext { + traverseAssets( + visit: GraphVisitor, + startAsset?: IAsset, + ): ?TContext { return this.#bundleGraph.traverseAssets( this.#bundle, mapVisitor(asset => assetFromValue(asset, this.#options), visit), + startAsset ? assetToAssetValue(startAsset) : undefined, ); } } diff --git a/packages/core/integration-tests/test/integration/scope-hoisting/es6/multiple-ancestors-wrap/index.js b/packages/core/integration-tests/test/integration/scope-hoisting/es6/multiple-ancestors-wrap/index.js new file mode 100644 index 00000000000..ebf8b39436c --- /dev/null +++ b/packages/core/integration-tests/test/integration/scope-hoisting/es6/multiple-ancestors-wrap/index.js @@ -0,0 +1,4 @@ +import value from './shouldBeWrapped'; +import otherValue from './wraps'; + +output = [value, otherValue]; diff --git a/packages/core/integration-tests/test/integration/scope-hoisting/es6/multiple-ancestors-wrap/shouldBeWrapped.js b/packages/core/integration-tests/test/integration/scope-hoisting/es6/multiple-ancestors-wrap/shouldBeWrapped.js new file mode 100644 index 00000000000..7a4e8a723a4 --- /dev/null +++ b/packages/core/integration-tests/test/integration/scope-hoisting/es6/multiple-ancestors-wrap/shouldBeWrapped.js @@ -0,0 +1 @@ +export default 42; diff --git a/packages/core/integration-tests/test/integration/scope-hoisting/es6/multiple-ancestors-wrap/wraps.js b/packages/core/integration-tests/test/integration/scope-hoisting/es6/multiple-ancestors-wrap/wraps.js new file mode 100644 index 00000000000..96d9fbd4023 --- /dev/null +++ b/packages/core/integration-tests/test/integration/scope-hoisting/es6/multiple-ancestors-wrap/wraps.js @@ -0,0 +1,5 @@ +import value from './shouldBeWrapped'; + +eval('void 0'); + +export default value + 1; diff --git a/packages/core/integration-tests/test/scope-hoisting.js b/packages/core/integration-tests/test/scope-hoisting.js index b1632bfc83e..39592f262c4 100644 --- a/packages/core/integration-tests/test/scope-hoisting.js +++ b/packages/core/integration-tests/test/scope-hoisting.js @@ -1003,6 +1003,27 @@ describe('scope hoisting', function () { assert.deepEqual(output, ['a', true]); }); + it('wraps an asset if any of its ancestors is wrapped, even if one is not', async function () { + let b = await bundle( + path.join( + __dirname, + '/integration/scope-hoisting/es6/multiple-ancestors-wrap/index.js', + ), + ); + + let contents = await outputFS.readFile( + b.getBundles()[0].filePath, + 'utf8', + ); + assert.strictEqual( + contents.match(/parcelRequire.register\(/g).length, + 2 /* once for parent asset, once for child wrapped asset */, + ); + + let output = await run(b); + assert.deepEqual(output, [42, 43]); + }); + it('supports importing from a wrapped asset with multiple bailouts', async function () { let b = await bundle( path.join( diff --git a/packages/core/types/index.js b/packages/core/types/index.js index 49b6d225741..d4def0daf83 100644 --- a/packages/core/types/index.js +++ b/packages/core/types/index.js @@ -1247,7 +1247,10 @@ export interface Bundle { /** Returns whether the bundle includes the given dependency. */ hasDependency(Dependency): boolean; /** Traverses the assets in the bundle. */ - traverseAssets(visit: GraphVisitor): ?TContext; + traverseAssets( + visit: GraphVisitor, + startAsset?: Asset, + ): ?TContext; /** Traverses assets and dependencies in the bundle. */ traverse( visit: GraphVisitor, diff --git a/packages/packagers/js/src/ScopeHoistingPackager.js b/packages/packagers/js/src/ScopeHoistingPackager.js index 0de4917c81e..c87451a3c19 100644 --- a/packages/packagers/js/src/ScopeHoistingPackager.js +++ b/packages/packagers/js/src/ScopeHoistingPackager.js @@ -247,7 +247,7 @@ export class ScopeHoistingPackager { async loadAssets(): Promise> { let queue = new PromiseQueue({maxConcurrent: 32}); let wrapped = []; - this.bundle.traverseAssets((asset, shouldWrap) => { + this.bundle.traverseAssets(asset => { queue.add(async () => { let [code, map] = await Promise.all([ asset.getCode(), @@ -257,7 +257,6 @@ export class ScopeHoistingPackager { }); if ( - shouldWrap || asset.meta.shouldWrap || this.isAsyncBundle || this.bundle.env.sourceType === 'script' || @@ -268,10 +267,25 @@ export class ScopeHoistingPackager { ) { this.wrappedAssets.add(asset.id); wrapped.push(asset); - return true; } }); + for (let wrappedAssetRoot of [...wrapped]) { + this.bundle.traverseAssets((asset, _, actions) => { + if (asset === wrappedAssetRoot) { + return; + } + + if (this.wrappedAssets.has(asset.id)) { + actions.skipChildren(); + return; + } + + this.wrappedAssets.add(asset.id); + wrapped.push(asset); + }, wrappedAssetRoot); + } + this.assetOutputs = new Map(await queue.run()); return wrapped; }