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

Dont merge isolated child assets #8527

Merged
merged 2 commits into from Oct 11, 2022
Merged

Dont merge isolated child assets #8527

merged 2 commits into from Oct 11, 2022

Conversation

AGawrys
Copy link
Contributor

@AGawrys AGawrys commented Oct 6, 2022

↪️ Pull Request

Fixes html element "examples" not rendering in markdown page. Problem was that two bundles of the same type were merged. One was isolated which we determined by dependency, and thus should not be merged ever.

I thought this whole branch

/**
* If this is an entry bundlegroup, we only allow one bundle per type in those groups
* So attempt to add the asset to the entry bundle if it's of the same type.
* This asset will be created by other dependency if it's in another bundlegroup
* and bundles of other types should be merged in the next step
*/

could be redundant code since we have a dedicated step to merging bundles of different types. However, it's not because we actually only attempt to merge bundles that have been created due to type change. So, entries are not considered in that, nor are any other code split bundles..... CC: @devongovett I think this is okay for now, but it looks like the defaultbundler maintains all bundles by type to determine where to merge. This seems like a big difference between the two bundlers that maybe needs to be fixed ?

💻 Examples

Before:
Screen Shot 2022-10-06 at 12 52 44 PM

After:
Screen Shot 2022-09-30 at 8 00 28 AM

🚨 Test instructions

✔️ PR Todo

  • Added/updated unit tests for this change
  • Tests
  • Tested on large app(s)

@parcel-benchmark
Copy link

parcel-benchmark commented Oct 6, 2022

Benchmark Results

Kitchen Sink ✅

Timings

Description Time Difference
Cold 1.78s -193.00ms 🚀
Cached 397.00ms -43.00ms 🚀

Cold Bundles

Bundle Size Difference Time Difference
dist/legacy/parcel.7cdb0fad.webp 102.94kb +0.00b 298.00ms -22.00ms 🚀
dist/legacy/parcel.7cdb0fad.webp 102.94kb +0.00b 299.00ms -23.00ms 🚀
dist/modern/parcel.7cdb0fad.webp 102.94kb +0.00b 299.00ms -23.00ms 🚀
dist/legacy/index.2c76ad23.js 1.66kb +0.00b 493.00ms -73.00ms 🚀
dist/legacy/index.8aaa89c9.js 1.20kb +0.00b 493.00ms -73.00ms 🚀
dist/modern/index.6be20f01.js 1.13kb +0.00b 492.00ms -74.00ms 🚀
dist/legacy/index.html 826.00b +0.00b 613.00ms -78.00ms 🚀
dist/modern/index.html 749.00b +0.00b 612.00ms -80.00ms 🚀
dist/legacy/index.b8ae99ba.css 94.00b +0.00b 305.00ms -23.00ms 🚀
dist/modern/index.31cedca9.css 94.00b +0.00b 304.00ms -24.00ms 🚀

Cached Bundles

No bundle changes detected.

React HackerNews 🚨

Timings

Description Time Difference
Cold FAILED -0.00ms
Cached FAILED -0.00ms

Cold Bundles

No bundles found, this is probably a failed build...

Cached Bundles

No bundles found, this is probably a failed build...

AtlasKit Editor 🚨

Timings

Description Time Difference
Cold FAILED -0.00ms
Cached FAILED -0.00ms

Cold Bundles

No bundles found, this is probably a failed build...

Cached Bundles

No bundles found, this is probably a failed build...

Three.js ✅

Timings

Description Time Difference
Cold 8.31s -115.00ms
Cached 380.00ms -80.00ms 🚀

Cold Bundles

No bundle changes detected.

Cached Bundles

No bundle changes detected.

Click here to view a detailed benchmark overview.

@AGawrys AGawrys merged commit e3a39a1 into v2 Oct 11, 2022
@devongovett devongovett deleted the agawrys/html-fix branch October 11, 2022 16:32
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.

None yet

3 participants