Deduplicate bundles following asset graph order (#4577)

packages/bundlers/default/src/DefaultBundler.js

@@ -216,11 +216,7 @@ export default new Bundler({
     });
 
     // Step 3: Remove assets that are duplicated in a parent bundle.
-    bundleGraph.traverseBundles({
-      exit(bundle) {
-        deduplicateBundle(bundleGraph, bundle);
-      },
-    });
+    deduplicate(bundleGraph);
 
     // Step 4: Find duplicated assets in different bundle groups, and separate them into their own parallel bundles.
     // If multiple assets are always seen together in the same bundles, combine them together.
@@ -361,9 +357,7 @@ export default new Bundler({
     }
 
     // Remove assets that are duplicated between shared bundles.
-    for (let sharedBundle of sharedBundles) {
-      deduplicateBundle(bundleGraph, sharedBundle);
-    }
+    deduplicate(bundleGraph);
 
     // Step 5: Mark async dependencies on assets that are already available in
     // the bundle as internally resolvable. This removes the dependency between
@@ -410,27 +404,24 @@ export default new Bundler({
   },
 });
 
-function deduplicateBundle(bundleGraph: MutableBundleGraph, bundle: Bundle) {
-  if (bundle.env.isIsolated() || !bundle.isSplittable) {
-    // If a bundle's environment is isolated, it can't access assets present
-    // in any ancestor bundles. Don't deduplicate any assets.
-    return;
-  }
-
-  bundle.traverse(node => {
-    if (node.type !== 'dependency') {
-      return;
-    }
-
-    let dependency = node.value;
-    let assets = bundleGraph.getDependencyAssets(dependency);
+function deduplicate(bundleGraph: MutableBundleGraph) {
+  bundleGraph.traverse(node => {
+    if (node.type === 'asset') {
+      let asset = node.value;
+      let bundles = bundleGraph.findBundlesWithAsset(asset);
+      for (let bundle of bundles) {
+        // If a bundle's environment is isolated, it can't access assets present
+        // in any ancestor bundles. Don't deduplicate any assets.
+        if (bundle.env.isIsolated() || !bundle.isSplittable) {
+          continue;
+        }
 
-    for (let asset of assets) {
-      if (
-        bundle.hasAsset(asset) &&
-        bundleGraph.isAssetReachableFromBundle(asset, bundle)
-      ) {
-        bundleGraph.removeAssetGraphFromBundle(asset, bundle);
+        if (
+          bundle.hasAsset(asset) &&
+          bundleGraph.isAssetReachableFromBundle(asset, bundle)
+        ) {
+          bundleGraph.removeAssetGraphFromBundle(asset, bundle);
+        }
       }
     }
   });

packages/core/integration-tests/test/integration/shared-sibling-scope-hoist/a.js

@@ -0,0 +1,4 @@
+import 'lodash/clone';
+import 'lodash/assign';
+
+export default 'a';

packages/core/integration-tests/test/integration/shared-sibling-scope-hoist/b.js

@@ -0,0 +1,4 @@
+import 'lodash/clone';
+import 'lodash/pick';
+
+export default 'b';

packages/core/integration-tests/test/integration/shared-sibling-scope-hoist/c.js

@@ -0,0 +1,4 @@
+import 'lodash/assign';
+import 'lodash/omit';
+
+export default 'c';

packages/core/integration-tests/test/integration/shared-sibling-scope-hoist/index.js

@@ -0,0 +1,5 @@
+output = Promise.all([
+  import('./a').then(mod => mod.default),
+  import('./b').then(mod => mod.default),
+  import('./c').then(mod => mod.default)
+]);

packages/core/integration-tests/test/scope-hoisting.js

@@ -2328,6 +2328,14 @@ describe('scope hoisting', function() {
     assert.deepEqual(await run(b), [3, 5]);
   });
 
+  it('deduplicates shared sibling assets between bundle groups', async () => {
+    let b = await bundle(
+      path.join(__dirname, '/integration/shared-sibling-scope-hoist/index.js'),
+    );
+
+    assert.deepEqual(await run(b), ['a', 'b', 'c']);
+  });
+
   it('can run an entry bundle whose entry asset is present in another bundle', async () => {
     let b = await bundle(
       ['index.js', 'value.js'].map(basename =>

packages/shared/scope-hoisting/src/generate.js

@@ -6,7 +6,6 @@ import type {
   ExpressionStatement,
   File,
   Statement,
-  StringLiteral,
 } from '@babel/types';
 
 import babelGenerate from '@babel/generator';
@@ -19,7 +18,6 @@ import template from '@babel/template';
 
 const REGISTER_TEMPLATE = template.statement<
   {|
-    ID: StringLiteral,
     REFERENCED_IDS: ArrayExpression,
     STATEMENTS: Array<Statement>,
   |},
@@ -30,7 +28,6 @@ const REGISTER_TEMPLATE = template.statement<
     STATEMENTS;
     $parcel$bundleWrapper._executed = true;
   }
-  parcelRequire.registerBundle(ID, $parcel$bundleWrapper);
   var $parcel$referencedAssets = REFERENCED_IDS;
   for (var $parcel$i = 0; $parcel$i < $parcel$referencedAssets.length; $parcel$i++) {
     parcelRequire.registerBundle($parcel$referencedAssets[$parcel$i], $parcel$bundleWrapper);
@@ -61,8 +58,7 @@ export function generate({
     interpreter = _interpreter;
   }
 
-  let entry = bundle.getMainEntry();
-  let isAsync = entry && !isEntry(bundle, bundleGraph);
+  let isAsync = !isEntry(bundle, bundleGraph);
 
   // Wrap async bundles in a closure and register with parcelRequire so they are executed
   // at the right time (after other bundle dependencies are loaded).
@@ -71,10 +67,11 @@ export function generate({
     statements = isAsync
       ? [
           REGISTER_TEMPLATE({
-            ID: t.stringLiteral(nullthrows(entry).id),
             STATEMENTS: statements,
             REFERENCED_IDS: t.arrayExpression(
-              [...referencedAssets].map(asset => t.stringLiteral(asset.id)),
+              [bundle.getMainEntry(), ...referencedAssets]
+                .filter(Boolean)
+                .map(asset => t.stringLiteral(asset.id)),
             ),
           }),
         ]