Multiple css bundles in Entry bundle groups issue

packages/bundlers/default/src/DefaultBundler.js

@@ -19,7 +19,13 @@ import {ContentGraph, Graph} from '@parcel/graph';
 import invariant from 'assert';
 import {ALL_EDGE_TYPES} from '@parcel/graph';
 import {Bundler} from '@parcel/plugin';
-import {setEqual, validateSchema, DefaultMap, BitSet} from '@parcel/utils';
+import {
+  setEqual,
+  setIntersect,
+  validateSchema,
+  DefaultMap,
+  BitSet,
+} from '@parcel/utils';
 import nullthrows from 'nullthrows';
 import {encodeJSONKeyComponent} from '@parcel/diagnostic';
 
@@ -625,6 +631,89 @@ function createIdealGraph(
           }
           if (!shouldMerge) continue;
           mergeBundle(nodeIdA, nodeIdB);
+        } else {
+          let overlap = new Set(bundleBbundleGroups);
+          setIntersect(overlap, bundleABundleGroups);
+          if (overlap.size > 0) {
+            // Two bundles are the same type and exist in the same bundle group but cannot be (fully) merged
+            // We must duplicate or create a new bundle in this case
+
+            let shouldCreateNewBundle =
+              overlap.size == bundleBbundleGroups.size ||
+              overlap.size == bundleABundleGroups.size
+                ? false
+                : true;
+            if (shouldCreateNewBundle) {
+              // Neither bundle can be the a host since both have unique groups
+              // So we may need to generate a new bundle for the intersection instead
+            } else {
+              // If the overlap of bundleGroups is a subset, then we should duplicate the bundle
+              // that results in a correct graph
+              let hostBundle =
+                overlap.size == bundleBbundleGroups.size
+                  ? [nodeIdB, b, bundleBbundleGroups]
+                  : [nodeIdA, a, bundleABundleGroups];
+              let duplicatedBundle =
+                overlap.size == bundleBbundleGroups.size
+                  ? [nodeIdA, a, bundleABundleGroups]
+                  : [nodeIdB, b, bundleBbundleGroups];
+              for (let asset of duplicatedBundle[1].assets) {
+                hostBundle[1].assets.add(asset);
+                hostBundle[1].size += asset.stats.size;
+              }
+              for (let group of overlap) {
+                let bundleGroup = nullthrows(bundleGraph.getNode(group));
+                invariant(
+                  bundleGroup != null && bundleGroup !== 'root',
+                  'Something went wrong with accessing a bundleGroup',
+                );
+                // Patch up dependency bundleGraph
+                for (let depId of dependencyBundleGraph.getNodeIdsConnectedTo(
+                  dependencyBundleGraph.getNodeIdByContentKey(
+                    String(duplicatedBundle[0]),
+                  ),
+                  ALL_EDGE_TYPES,
+                )) {
+                  let depNode = dependencyBundleGraph.getNode(depId);
+                  invariant(bundleGroup.mainEntryAsset != null);
+                  if (
+                    depNode &&
+                    depNode.type === 'dependency' &&
+                    depNode.value.sourcePath ==
+                      bundleGroup.mainEntryAsset.filePath
+                  ) {
+                    dependencyBundleGraph.addEdge(
+                      depId,
+                      dependencyBundleGraph.getNodeIdByContentKey(
+                        String(hostBundle[0]),
+                      ),
+                      dependencyPriorityEdges.parallel,
+                    );
+
+                    dependencyBundleGraph.removeEdge(
+                      depId,
+                      dependencyBundleGraph.getNodeIdByContentKey(
+                        String(duplicatedBundle[0]),
+                      ),
+                      dependencyPriorityEdges.parallel,
+                    );
+                    for (let asset of duplicatedBundle[1].assets) {
+                      replaceAssetReference(
+                        asset,
+                        duplicatedBundle[1],
+                        hostBundle[1],
+                        depNode.value,
+                      );
+                    }
+                  }
+                }
+                // This might be a referencing bundle, not necessarily the group, so we
+                detachFromBundleGroup(group, duplicatedBundle[0]);
+              }
+            }
+            //We might've changed the bundleGroups of A, which should be recalculated;
+            bundleABundleGroups = getBundleGroupsForBundle(nodeIdA);
+          }
         }
       }
     }
@@ -1141,7 +1230,24 @@ function createIdealGraph(
       );
     }
   }
-
+  function detachFromBundleGroup(groupId: NodeId, bundleId: NodeId) {
+    // This removes a particular bundle from the specfied bundleGroup
+    if (bundleGraph.hasEdge(groupId, bundleId)) {
+      bundleGraph.removeEdge(groupId, bundleId);
+    } else {
+      let referencingBundleId;
+      bundleGraph.traverse(nodeId => {
+        if (bundleGraph.hasEdge(nodeId, bundleId)) {
+          referencingBundleId = nodeId;
+        }
+      }, groupId);
+      invariant(
+        referencingBundleId != null,
+        'Expected to remove bundle from group but could not find it...',
+      );
+      bundleGraph.removeEdge(referencingBundleId, bundleId);
+    }
+  }
   function deleteBundle(bundleRoot: BundleRoot) {
     bundleGraph.removeNode(nullthrows(bundles.get(bundleRoot.id)));
     bundleRoots.delete(bundleRoot);
@@ -1234,11 +1340,17 @@ function createIdealGraph(
     bundleRoot: BundleRoot,
     toReplace: Bundle,
     replaceWith: Bundle,
+    dependency?: Dependency,
   ): void {
     let replaceAssetReference = assetReference.get(bundleRoot).map(entry => {
       let bundle = entry[1];
+      let dep = entry[0];
       if (bundle == toReplace) {
-        return [entry[0], replaceWith];
+        if (dependency && dependency == dep) {
+          return [entry[0], replaceWith];
+        } else if (dependency == null) {
+          return [entry[0], replaceWith];
+        }
       }
       return entry;
     });

packages/core/integration-tests/test/css-modules.js

@@ -547,7 +547,7 @@ describe('css modules', () => {
       },
     ]);
   });
-  // Forked because experimental bundler will not merge bundles of same types if they do not share all their bundlegroups
+
   it('should handle @import in css modules', async function () {
     let b = await bundle(
       [
@@ -608,7 +608,7 @@ describe('css modules', () => {
       },
       {
         type: 'css',
-        assets: ['a.module.css', 'b.module.css'],
+        assets: ['a.module.css', 'b.module.css', 'index.module.css'], //We duplicate index.module.css to maintain one css bundle per entry bundleGroup
       },
       {
         type: 'css',

packages/core/integration-tests/test/css.js

@@ -68,7 +68,59 @@ describe('css', () => {
         css.indexOf('.e {') < css.indexOf('.a {'),
     );
   });
+  it('should place one css bundle per bundlegroup for naming reasons', async () => {
+    let b = await bundle(
+      path.join(__dirname, '/integration/multi-css-bug/src/entry.js'),
+    );
+
+    assertBundles(b, [
+      {
+        name: 'entry.js',
+        type: 'js',
+        assets: [
+          'bundle-url.js',
+          'cacheLoader.js',
+          'css-loader.js',
+          'entry.js',
+          'js-loader.js',
+        ],
+      },
+      {
+        type: 'js',
+        assets: ['esmodule-helpers.js', 'index.js'],
+      },
+      {name: 'Foo.css', type: 'css', assets: ['foo.css']},
+      {name: 'entry.css', type: 'css', assets: ['foo.css', 'main.css']},
+    ]);
+  });
+  it.skip('create a new css bundle to maintain one css bundle per bundlegroup constraint', async () => {
+    let b = await bundle(
+      path.join(
+        __dirname,
+        '/integration/multi-css-multi-entry-bug/src/entry.js',
+      ),
+    );
 
+    assertBundles(b, [
+      {
+        name: 'entry.js',
+        type: 'js',
+        assets: [
+          'bundle-url.js',
+          'cacheLoader.js',
+          'css-loader.js',
+          'entry.js',
+          'js-loader.js',
+        ],
+      },
+      {
+        type: 'js',
+        assets: ['esmodule-helpers.js', 'index.js'],
+      },
+      {name: 'Foo.css', type: 'css', assets: ['foo.css']},
+      {name: 'entry.css', type: 'css', assets: ['foo.css', 'main.css']},
+    ]);
+  });
   it('should support loading a CSS bundle along side dynamic imports', async () => {
     let b = await bundle(
       path.join(__dirname, '/integration/dynamic-css/index.js'),

packages/core/integration-tests/test/html.js

@@ -2213,7 +2213,7 @@ describe('html', function () {
       },
       {
         type: 'css',
-        assets: ['shared.css'],
+        assets: ['shared.css', 'other.css'], //We've duplicated 'other' to maintain 1 css bundle per entry
       },
     ]);
 
@@ -2309,7 +2309,7 @@ describe('html', function () {
       2,
     );
 
-    // a.html should reference a.js only
+    // a.html should reference a.js only, but references a css bundle with BOTH a.css and b.css
     assert.equal(html.match(/a\.[a-z0-9]+\.js/g).length, 1);
 
     assert.equal(html.match(/b\.[a-z0-9]+\.js/g), null);
@@ -2319,7 +2319,7 @@ describe('html', function () {
       'utf8',
     );
     assert(css.includes('.a {'));
-    assert(!css.includes('.b {'));
+    assert(css.includes('.b {'));
 
     // b.html should point to a CSS bundle containing only b.css
     // It should not point to the bundle containing a.css from a.html

packages/core/integration-tests/test/integration/multi-css-bug/src/Foo/foo.css

@@ -0,0 +1,3 @@
+.foo {
+  outline: 1px solid red;
+}

packages/core/integration-tests/test/integration/multi-css-bug/src/Foo/index.js

@@ -0,0 +1,3 @@
+import './foo.css';
+
+export default function () { return 'foo' };

packages/core/integration-tests/test/integration/multi-css-bug/src/entry.js

@@ -0,0 +1,35 @@
+// This repro was taken from https://github.com/parcel-bundler/parcel/issues/8813
+
+// This file needs to import a CSS asset, so that `index.css` will be created.
+import './main.css';
+
+/*
+   Import the CSS out of the package; causes a bundle Group entry type error.
+   This import causes the asset in the lazy imported bundle below to also be
+   named `index.css`.
+*/
+import './Foo/foo.css';
+
+/*
+   Due to the import above, the dynamic import CSS asset is also created as
+   `index.css`, resulting in a name collision with the `index.css` asset created
+   for this file.
+
+   If the import of 'foo.css' above is removed, the dynamic import CSS asset
+   will be named `Foo.<hash>.css` as expected, like its JS file.
+
+   If the import of 'main.css' above is removed, the dynamic import CSS asset
+   will be named `index.css` as expected, but there will be no name collision
+   because this file did not generate an `index.css` asset itself.
+
+   Also, if parcel is run with a cache, on the first execution the
+   AssertionError for the bundle group will be raised.  However, on a second
+   execution, the AssertError will not occur, but the generated `index.css` will
+   only contain the content of `foo.css` and be missing the content of
+   `main.css`.
+
+   In a React app, the dynamic import occurs via React.lazy:
+     import {lazy} from 'react'
+     const foo = lazy(() => import('./Foo'));
+*/
+import('./Foo');

packages/core/integration-tests/test/integration/multi-css-bug/src/main.css

@@ -0,0 +1,3 @@
+.main {
+  outline: 1px solid blue;
+}

packages/core/integration-tests/test/integration/multi-css-multi-entry-bug/src/Foo/foo.css

@@ -0,0 +1,3 @@
+.foo {
+  outline: 1px solid red;
+}

packages/core/integration-tests/test/integration/multi-css-multi-entry-bug/src/Foo/index.js

@@ -0,0 +1,3 @@
+import './foo.css';
+
+export default function () { return 'foo' };

packages/core/integration-tests/test/integration/multi-css-multi-entry-bug/src/entry.js

@@ -0,0 +1,8 @@
+// This repro was taken from https://github.com/parcel-bundler/parcel/issues/8813
+
+import './main.css';
+
+import './Foo/foo.css';
+
+import('./Foo');
+import('./index-sib');

packages/core/integration-tests/test/integration/multi-css-multi-entry-bug/src/index-sib.js

@@ -0,0 +1,2 @@
+
+import './main.css';

packages/core/integration-tests/test/integration/multi-css-multi-entry-bug/src/main.css

@@ -0,0 +1,3 @@
+.main {
+  outline: 1px solid blue;
+}

packages/namers/default/src/DefaultNamer.js

@@ -31,7 +31,7 @@ export default (new Namer({
       assert(
         entryBundlesOfType.length === 1,
         // Otherwise, we'd end up naming two bundles the same thing.
-        'Bundle group cannot have more than one entry bundle of the same type',
+        `Bundle group cannot have more than one entry bundle of the same type. The offending bundle type is ${entryBundlesOfType[0].type}`,
       );
     }