Multiple css bundles in Entry bundle groups issue #9023

AGawrys · 2023-05-17T19:36:37Z

↪️ Pull Request

This PR aims to fix #8813 , or, the issue of having multiple css/ type change bundles in one entry bundlegroup. Currently we only merge if those bundles share the same bundleGroups, this isn't perfect because it may result in sibling type change bundles within one bundlegroup.

The solution below duplicates assets of the "offending" bundle in favor of maintaining the "1 type change bundle per bundlegroup" rule, and does so based on the overlap of bundleGroups.

~~This changes the current behavior in a non-significant way, so I have a couple questions about if this change is okay, specifically for the affected test cases.~~

Update Tests now align with v2 as we don't merge or duplicate assets if not needsStableName

💻 Examples & Questions

Here are the graphs of the reproduction from issue #8813:

Before:
AssertionError [ERR_ASSERTION]: Bundle group cannot have more than one entry bundle of the same type
After:

✔️ PR Todo

Added/updated unit tests for this change
Add test case mentioned in question 3 & Potentially expand feature to support scenario( where shared css sibling bundle can neither be merged nor duplicated, i.e. each shares a bundle group, but also each have their own unique bundle groups)
Included links to related issues/PRs

Update:

Only merging in the case of, needsStableName bundles, and only duplicating css assets iff they cannot be merged.
Affected Test cases have been reverted since they no longer meet the conditions of needsStableName for merging

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

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

…of bundlegroups, revert test cases

parcel-benchmark · 2023-08-14T18:13:03Z

Benchmark Results

Kitchen Sink ✅

Timings

Description	Time	Difference
Cold	1.49s	+52.00ms
Cached	269.00ms	+2.00ms

Cold Bundles

Bundle	Size	Difference	Time	Difference
dist/legacy/index.html	826.00b	+0.00b	418.00ms	+21.00ms ⚠️
dist/legacy/index.b8ae99ba.css	94.00b	+0.00b	290.00ms	+28.00ms ⚠️
dist/modern/index.31cedca9.css	94.00b	+0.00b	289.00ms	+28.00ms ⚠️

Cached Bundles

Bundle	Size	Difference	Time	Difference
dist/legacy/parcel.7cdb0fad.webp	102.94kb	+0.00b	251.00ms	-17.00ms 🚀
dist/legacy/index.b8ae99ba.css	94.00b	+0.00b	260.00ms	-21.00ms 🚀
dist/modern/index.31cedca9.css	94.00b	+0.00b	260.00ms	-21.00ms 🚀

React HackerNews ✅

Timings

Description	Time	Difference
Cold	4.01s	-15.00ms
Cached	419.00ms	-7.00ms

Cold Bundles

Bundle	Size	Difference	Time	Difference
dist/logo.8dd07848.png	244.00b	+0.00b	232.00ms	-15.00ms 🚀

Cached Bundles

Bundle	Size	Difference	Time	Difference
dist/logo.8dd07848.png	244.00b	+0.00b	265.00ms	+25.00ms ⚠️

AtlasKit Editor ✅

Timings

Description	Time	Difference
Cold	35.91s	-709.00ms
Cached	2.39s	+30.00ms

Cold Bundles

Bundle	Size	Difference	Time	Difference
dist/component-lazy.aeb22f50.js	59.50kb	+0.00b	5.82s	-511.00ms 🚀
dist/codeViewerRenderer.7d374cd5.js	2.61kb	+0.00b	8.85s	-3.04s 🚀
dist/ro.ee42c980.js	478.00b	+0.00b	6.47s	-1.91s 🚀

Cached Bundles

Bundle	Size	Difference	Time	Difference
dist/media-viewer.bc1a2415.js	537.32kb	+0.00b	12.38s	+689.00ms ⚠️
dist/EmojiPickerComponent.c199902f.js	189.68kb	+0.00b	12.40s	+640.00ms ⚠️
dist/ConfigPanelFieldsLoader.1a016f33.js	82.96kb	+0.00b	12.40s	+640.00ms ⚠️
dist/pdfRenderer.6335b9a2.js	12.08kb	+0.00b	12.38s	+3.64s ⚠️
dist/index.html	248.00b	+0.00b	6.30s	-5.63s 🚀

Three.js ✅

Timings

Description	Time	Difference
Cold	2.95s	-25.00ms
Cached	323.00ms	-13.00ms

Cold Bundles

No bundle changes detected.

Cached Bundles

No bundle changes detected.

Click here to view a detailed benchmark overview.

devongovett · 2023-08-15T18:03:51Z

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

+      {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 () => {


Oh forgot about this... okay yes so I still do have one more question. The current solution works with our existing test cases and fixes the cases in the mentioned issue. However, I added this case as an alternative and I'm not sure what our best move is. No one has encountered this to my knowledge yet.

In this test case, we have 1 entry (entry.js) with two async js siblings, with which it shared two other .css files (main and foo.css )

Both foo.css (3) and main.css (2) have needsStableName: true but have no subset or perfect overlap in terms of bundleGroups. Should we over-fetch and merge them anyway or should we create a new bundle with duplicated just for entry.js?

AFAIK old bundler just inefficiently merged them.

cc: @devongovett

jondlm · 2023-10-04T19:54:22Z

I ran into this one again today with another codebase. Seems like maybe we should move forward with the fix even though we don't have a great solution to the skiped test?

jondlm · 2023-10-13T23:17:40Z

@AGawrys I was testing out this patch today and noticed that BitSet here has been moved to @parcel/graph. It's being double imported and failing. I simply removed the import from @parcel/util and it seems to work now.

jondlm · 2023-10-13T23:42:08Z

Darn. Tested this patch out on our codebase and I'm at least getting a slightly more helpful error message :)

🚨 Build failed.

@parcel/namer-default: Bundle group cannot have more than one entry bundle of the same type. The offending bundle type is css

I'll dig into it more next week.

TobiasSchikora · 2023-11-02T20:51:14Z

Any updates on this? :-/

jondlm · 2023-11-02T21:08:21Z

Sorry forgot to circle back on this. Unfortunately that patch didn't fix the issue so I'm still hunting for a proper fix.

TobiasSchikora · 2023-11-03T10:10:54Z

It seems to be a very hard bug. Thank you anyway for your efforts. I think you would help many people who want to update from Parcel 1 to Parcel 2 but can't because of this bug, including me. I also notice that using Parcel 1 is slowly becoming a problem. This can't go on for long and I don't want to switch to Vite or anything else because I really like Parcel. I hope a solution can be found! Thank you!

AGawrys · 2023-12-12T19:08:08Z

@jondlm @TobiasSchikora So sorry that this has been such a pain for you. I updated the branch, removing the extra bitSet import seemed to work for me.

@jondlm In this PR there is a scenario I wasn't sure how to cover, it could be what you're running into? i.e. A new scenario with the same symptom. Do you have a public repro or minimal repro I could take a look at so I could confirm? I can work to expand this fix if my hunch is correct.

In the meantime I'll try to get this merged today.

jondlm

LGTM. I tested it out with one of my repros and it fixed the issue. I still think there are some lurking issues with duplicate css bundles but this seems like a step in the right direction.

Thank you @AGawrys!

AGawrys · 2024-01-18T17:51:32Z

@jondlm
Sorry, I had to formally request you as a reviewer for your approval to allow me to merge, mind approving once more? Thanks

jondlm

Sure thing!

jondlm · 2024-01-18T17:54:44Z

Oh. I don't have write access to the repo so my review probably doesn't count :(

mischnic

There you go 😄

TobiasSchikora · 2024-01-20T07:47:48Z

WOW, that's really great news. I can't wait to try it out! Thank you very much! 🧡

TobiasSchikora · 2024-01-20T08:29:19Z

OK, I have now installed the latest Nightly (2.11.1-nightly.3109). The original error message is gone, but I have a new one:

🚨 Build failed.

@parcel/bundler-default: Edge from 289 to 83 not found!

  Error: Edge from 289 to 83 not found!
      at ContentGraph.removeEdge (C:\mf\node_modules\@parcel\graph\lib\Graph.js:117:13)
      at createIdealGraph (C:\mf\node_modules\@parcel\bundler-default\lib\DefaultBundler.js:648:43)
      at Object.bundle (C:\mf\node_modules\@parcel\bundler-default\lib\DefaultBundler.js:123:19)
      at BundlerRunner.bundle (C:\mf\node_modules\@parcel\core\lib\requests\BundleGraphRequest.js:261:23)
      at async Object.run (C:\mf\node_modules\@parcel\core\lib\requests\BundleGraphRequest.js:129:17)
      at async RequestTracker.runRequest (C:\mf\node_modules\@parcel\core\lib\RequestTracker.js:653:20)
      at async Object.run (C:\mf\node_modules\@parcel\core\lib\requests\ParcelBuildRequest.js:52:7)
      at async RequestTracker.runRequest (C:\mf\node_modules\@parcel\core\lib\RequestTracker.js:653:20)
      at async Parcel._build (C:\mf\node_modules\@parcel\core\lib\Parcel.js:315:11)
      at async Parcel._startNextBuild (C:\mf\node_modules\@parcel\core\lib\Parcel.js:223:24)

Does anyone know what to do there? Thanks for any help!

* upstream/v2: (22 commits) Add source map support to the inline-require optimizer (#9511) [Web Extension] Add content script world property to manifest schema validation (#9510) feat: add getCurrentPackageManager (#9505) Default Bundler Contributor Notes (#9488) rename parentAsset to root for msb config and remove unstable (#9486) Macro errors -> v2 (#9501) Statically evaluate constants referenced by macros (#9487) Multiple css bundles in Entry bundle groups issue (#9023) Fix macro issues (#9485) Bump follow-redirects from 1.14.7 to 1.15.4 (#9475) Revert more CI changes to centos job (#9472) Use lightningcss to implement CSS packager (#8492) Fixup CI again (#9471) Clippy and use napi's Either3 (#9047) Upgrade to eslint 8 (#8580) Add support for JS macros (#9299) Fixup REPL CI (#9467) Drop per-pipeline transformation cache (#9459) Upgrade some CI actions (#9466) REPL (#9365) ...

AGawrys added 2 commits May 11, 2023 09:49

initial attempt to fix

3997714

remedy resulting failing tests

1c17b66

AGawrys requested review from devongovett and mattcompiles May 17, 2023 19:36

Merge branch 'v2' into agawrys/multi-css-in-entry-bug

f69e2cb

AGawrys commented May 17, 2023

View reviewed changes

packages/core/integration-tests/test/css-modules.js Outdated Show resolved Hide resolved

add alternative test case

09fd358

devongovett reviewed Jun 7, 2023

View reviewed changes

packages/core/integration-tests/test/html.js Outdated Show resolved Hide resolved

This was referenced Jul 24, 2023

Improve the handling of entry bundle collisions #8813

Closed

[Parcel 2]: Bundle group cannot have more than one entry bundle of the same type #6310

Open

AGawrys added 2 commits August 14, 2023 10:36

only duplicate css in case of needstablename and non perfect overlap …

3952f23

…of bundlegroups, revert test cases

Merge branch 'v2' into agawrys/multi-css-in-entry-bug

a6a4f84

AGawrys requested a review from devongovett August 14, 2023 17:50

devongovett reviewed Aug 15, 2023

View reviewed changes

AGawrys added 2 commits August 23, 2023 11:44

Merge branch 'v2' into agawrys/multi-css-in-entry-bug

a7fe037

Merge branch 'v2' into agawrys/multi-css-in-entry-bug

93397dd

AGawrys requested a review from devongovett August 25, 2023 17:32

Merge branch 'v2' into agawrys/multi-css-in-entry-bug

72db3bc

Merge branch 'v2' into agawrys/multi-css-in-entry-bug

342b2ec

AGawrys added 2 commits December 12, 2023 10:56

remote bitset import

ccb1b7b

Merge branch 'v2' into agawrys/multi-css-in-entry-bug

c618ea9

Merge branch 'v2' into agawrys/multi-css-in-entry-bug

abf2ea4

AGawrys added 7 commits December 13, 2023 09:49

Merge branch 'v2' into agawrys/multi-css-in-entry-bug

5f1b78b

Merge branch 'v2' into agawrys/multi-css-in-entry-bug

a5a711c

Merge branch 'v2' into agawrys/multi-css-in-entry-bug

d5f07e4

Merge branch 'v2' into agawrys/multi-css-in-entry-bug

c948792

Merge branch 'v2' into agawrys/multi-css-in-entry-bug

0c98c83

Merge branch 'v2' into agawrys/multi-css-in-entry-bug

3092c04

Merge branch 'v2' into agawrys/multi-css-in-entry-bug

850e46a

AGawrys mentioned this pull request Jan 14, 2024

Default Bundler Contributor Notes #9488

Merged

3 tasks

Merge branch 'v2' into agawrys/multi-css-in-entry-bug

0ca2488

jondlm approved these changes Jan 17, 2024

View reviewed changes

AGawrys requested a review from jondlm January 18, 2024 17:50

jondlm approved these changes Jan 18, 2024

View reviewed changes

mischnic approved these changes Jan 18, 2024

View reviewed changes

AGawrys merged commit 6bebe54 into v2 Jan 18, 2024
9 of 16 checks passed

AGawrys deleted the agawrys/multi-css-in-entry-bug branch January 18, 2024 19:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Multiple css bundles in Entry bundle groups issue #9023

Multiple css bundles in Entry bundle groups issue #9023

AGawrys commented May 17, 2023 •

edited

parcel-benchmark commented Aug 14, 2023 •

edited

Timings

Cold Bundles

Cached Bundles

Timings

Cold Bundles

Cached Bundles

Timings

Cold Bundles

Cached Bundles

Timings

Cold Bundles

Cached Bundles

devongovett Aug 15, 2023

AGawrys Aug 15, 2023 •

edited

jondlm commented Oct 4, 2023

jondlm commented Oct 13, 2023

jondlm commented Oct 13, 2023

TobiasSchikora commented Nov 2, 2023

jondlm commented Nov 2, 2023

TobiasSchikora commented Nov 3, 2023

AGawrys commented Dec 12, 2023

jondlm left a comment

AGawrys commented Jan 18, 2024

jondlm left a comment

jondlm commented Jan 18, 2024

mischnic left a comment

TobiasSchikora commented Jan 20, 2024

TobiasSchikora commented Jan 20, 2024

Multiple css bundles in Entry bundle groups issue #9023

Multiple css bundles in Entry bundle groups issue #9023

Conversation

AGawrys commented May 17, 2023 • edited

↪️ Pull Request

💻 Examples & Questions

✔️ PR Todo

Update:

parcel-benchmark commented Aug 14, 2023 • edited

Benchmark Results

Timings

Cold Bundles

Cached Bundles

Timings

Cold Bundles

Cached Bundles

Timings

Cold Bundles

Cached Bundles

Timings

Cold Bundles

Cached Bundles

devongovett Aug 15, 2023

Choose a reason for hiding this comment

AGawrys Aug 15, 2023 • edited

Choose a reason for hiding this comment

jondlm commented Oct 4, 2023

jondlm commented Oct 13, 2023

jondlm commented Oct 13, 2023

TobiasSchikora commented Nov 2, 2023

jondlm commented Nov 2, 2023

TobiasSchikora commented Nov 3, 2023

AGawrys commented Dec 12, 2023

jondlm left a comment

Choose a reason for hiding this comment

AGawrys commented Jan 18, 2024

jondlm left a comment

Choose a reason for hiding this comment

jondlm commented Jan 18, 2024

mischnic left a comment

Choose a reason for hiding this comment

TobiasSchikora commented Jan 20, 2024

TobiasSchikora commented Jan 20, 2024

AGawrys commented May 17, 2023 •

edited

parcel-benchmark commented Aug 14, 2023 •

edited

AGawrys Aug 15, 2023 •

edited