Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Multiple css bundles in Entry bundle groups issue #9023

Merged
merged 21 commits into from
Jan 18, 2024

Conversation

AGawrys
Copy link
Contributor

@AGawrys AGawrys commented May 17, 2023

↪️ 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

@parcel-benchmark
Copy link

parcel-benchmark commented Aug 14, 2023

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.

{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 () => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

skipped?

Copy link
Contributor Author

@AGawrys AGawrys Aug 15, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

neithercanhost
cc: @devongovett

@jondlm
Copy link
Contributor

jondlm commented Oct 4, 2023

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
Copy link
Contributor

jondlm commented Oct 13, 2023

@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
Copy link
Contributor

jondlm commented Oct 13, 2023

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
Copy link

Any updates on this? :-/

@jondlm
Copy link
Contributor

jondlm commented Nov 2, 2023

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
Copy link

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
Copy link
Contributor Author

AGawrys commented Dec 12, 2023

@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.

@AGawrys AGawrys mentioned this pull request Jan 14, 2024
3 tasks
Copy link
Contributor

@jondlm jondlm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 AGawrys requested a review from jondlm January 18, 2024 17:50
@AGawrys
Copy link
Contributor Author

AGawrys commented Jan 18, 2024

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

Copy link
Contributor

@jondlm jondlm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure thing!

@jondlm
Copy link
Contributor

jondlm commented Jan 18, 2024

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

Copy link
Member

@mischnic mischnic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There you go 😄

@AGawrys AGawrys merged commit 6bebe54 into v2 Jan 18, 2024
9 of 16 checks passed
@AGawrys AGawrys deleted the agawrys/multi-css-in-entry-bug branch January 18, 2024 19:01
@TobiasSchikora
Copy link

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

@TobiasSchikora
Copy link

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!

lettertwo added a commit that referenced this pull request Jan 30, 2024
* 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)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve the handling of entry bundle collisions
6 participants