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

HMR: Don't wait for packaging #8582

Merged
merged 6 commits into from Nov 3, 2022
Merged

HMR: Don't wait for packaging #8582

merged 6 commits into from Nov 3, 2022

Conversation

mischnic
Copy link
Member

We may or may not need this bailout logic: 2acf9c5

Some more discussions about bailing out: #4143 (comment)

Closes #4143

@mischnic mischnic marked this pull request as draft October 29, 2022 19:38
@parcel-benchmark
Copy link

parcel-benchmark commented Oct 29, 2022

Benchmark Results

Kitchen Sink ✅

Timings

Description Time Difference
Cold 2.24s +232.00ms ⚠️
Cached 491.00ms +84.00ms ⚠️

Cold Bundles

Bundle Size Difference Time Difference
dist/legacy/parcel.7cdb0fad.webp 102.94kb +0.00b 126.00ms -180.00ms 🚀
dist/legacy/parcel.7cdb0fad.webp 102.94kb +0.00b 127.00ms -180.00ms 🚀
dist/modern/parcel.7cdb0fad.webp 102.94kb +0.00b 126.00ms -181.00ms 🚀
dist/legacy/index.2c76ad23.js 1.66kb +0.00b 604.00ms +78.00ms ⚠️
dist/legacy/index.8aaa89c9.js 1.20kb +0.00b 604.00ms +77.00ms ⚠️
dist/modern/index.6be20f01.js 1.13kb +0.00b 604.00ms +78.00ms ⚠️
dist/legacy/index.html 826.00b +0.00b 820.00ms +146.00ms ⚠️
dist/modern/index.html 749.00b +0.00b 818.00ms +147.00ms ⚠️
dist/legacy/index.b8ae99ba.css 94.00b +0.00b 366.00ms +47.00ms ⚠️
dist/modern/index.31cedca9.css 94.00b +0.00b 365.00ms +47.00ms ⚠️

Cached Bundles

Bundle Size Difference Time Difference
dist/legacy/parcel.7cdb0fad.webp 102.94kb +0.00b 377.00ms +24.00ms ⚠️
dist/legacy/parcel.7cdb0fad.webp 102.94kb +0.00b 381.00ms +27.00ms ⚠️
dist/modern/parcel.7cdb0fad.webp 102.94kb +0.00b 382.00ms +28.00ms ⚠️
dist/legacy/index.b8ae99ba.css 94.00b +0.00b 390.00ms +28.00ms ⚠️
dist/modern/index.31cedca9.css 94.00b +0.00b 389.00ms +29.00ms ⚠️

React HackerNews ✅

Timings

Description Time Difference
Cold 13.69s +516.00ms
Cached 510.00ms -77.00ms 🚀

Cold Bundles

No bundle changes detected.

Cached Bundles

Bundle Size Difference Time Difference
dist/index.js 486.37kb +0.00b 6.48s -769.00ms 🚀
dist/PermalinkedComment.46b19af5.js 4.21kb +0.00b 6.48s -770.00ms 🚀
dist/UserProfile.f8cd7884.js 1.57kb +0.00b 6.48s -769.00ms 🚀
dist/NotFound.960ab92b.js 429.00b +0.00b 6.48s -769.00ms 🚀
dist/logo.c5bb83f1.png 246.00b +0.00b 6.48s -777.00ms 🚀

AtlasKit Editor ✅

Timings

Description Time Difference
Cold 1.98m -2.00s
Cached 2.90s +355.00ms ⚠️

Cold Bundles

No bundle changes detected.

Cached Bundles

No bundle changes detected.

Three.js ✅

Timings

Description Time Difference
Cold 8.65s +729.00ms ⚠️
Cached 315.00ms -56.00ms 🚀

Cold Bundles

No bundle changes detected.

Cached Bundles

Bundle Size Difference Time Difference
dist/Three.js 580.02kb +0.00b 5.66s -1.00s 🚀

Click here to view a detailed benchmark overview.

@devongovett
Copy link
Member

We should do some testing, but I think this should be ok because the dev server blocks requests until a build is complete. So if an async dep got introduced and requested immediately, it should work.

@devongovett
Copy link
Member

I noticed in profiles that createWriteBundlesRequest was still running before the HMR update was actually emitted, due to it being an async function that wasn't awaited. Since createWriteBundlesRequest can still be pretty slow due to traversing the bundle graph and hashing, I added awaiting to the report call so we ensure the HMR update is emitted first.

@mischnic mischnic marked this pull request as ready for review November 1, 2022 23:57
Copy link
Member

@devongovett devongovett left a comment

Choose a reason for hiding this comment

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

Did some testing and it seems ok to me!

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.

Feature: Faster Than Light (FTL) reloads
3 participants