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

Apply HMR updates in topological order #8752

Merged
merged 4 commits into from
Jan 7, 2023
Merged

Apply HMR updates in topological order #8752

merged 4 commits into from
Jan 7, 2023

Conversation

devongovett
Copy link
Member

This fixes an issue with circular dependencies and HMR. One example is if you had a React context provider, and a consumer that relied on it that was also a dependency of the provider, the context would become null after saving either of these components causing a crash.

In the test example, both of these components do not self accept the update because they don't only export React components (one exports a context, the other exports another function). Therefore, the update bubbles to the root App component which does accept it. However, the other modules still must be re-executed so they use the updated component. But since the dependency is circular, consumer.js re-executes first, and then provider.js. So that means consumer.js is still pointing at an old version of provider.js with the old context, which no longer has a value assigned to it.

This PR fixes the issue by splitting the HMR accept phase into two:

  1. Dispose all of the modules. Keep track of which ones previously accepted updates.
  2. Re-execute only the modules that previously accepted, and run their accept callbacks.

Since we only re-execute the root-level accepting assets, but their children are already disposed, the children will be re-executed in topological order (the same as when first loading the asset). Splitting this into two phases also matches webpack's behavior more closely.

@parcel-benchmark
Copy link

parcel-benchmark commented Jan 5, 2023

Benchmark Results

Kitchen Sink ✅

Timings

Description Time Difference
Cold 1.47s -25.00ms
Cached 343.00ms +25.00ms ⚠️

Cold Bundles

Bundle Size Difference Time Difference
dist/legacy/parcel.7cdb0fad.webp 102.94kb +0.00b 83.00ms -16.00ms 🚀
dist/legacy/parcel.7cdb0fad.webp 102.94kb +0.00b 83.00ms -17.00ms 🚀
dist/modern/parcel.7cdb0fad.webp 102.94kb +0.00b 83.00ms -17.00ms 🚀
dist/legacy/index.2c76ad23.js 1.66kb +0.00b 405.00ms -33.00ms 🚀
dist/legacy/index.8aaa89c9.js 1.20kb +0.00b 405.00ms -33.00ms 🚀
dist/modern/index.6be20f01.js 1.13kb +0.00b 404.00ms -35.00ms 🚀
dist/legacy/index.html 826.00b +0.00b 505.00ms -36.00ms 🚀
dist/modern/index.html 749.00b +0.00b 504.00ms -36.00ms 🚀
dist/legacy/index.b8ae99ba.css 94.00b +0.00b 239.00ms -32.00ms 🚀
dist/modern/index.31cedca9.css 94.00b +0.00b 239.00ms -32.00ms 🚀

Cached Bundles

Bundle Size Difference Time Difference
dist/legacy/parcel.7cdb0fad.webp 102.94kb +0.00b 254.00ms +145.00ms ⚠️
dist/legacy/parcel.7cdb0fad.webp 102.94kb +0.00b 256.00ms +148.00ms ⚠️
dist/modern/parcel.7cdb0fad.webp 102.94kb +0.00b 256.00ms +147.00ms ⚠️

React HackerNews ✅

Timings

Description Time Difference
Cold 9.25s -113.00ms
Cached 431.00ms +3.00ms

Cold Bundles

No bundle changes detected.

Cached Bundles

No bundle changes detected.

AtlasKit Editor ✅

Timings

Description Time Difference
Cold 1.54m +1.65s
Cached 2.16s +7.00ms

Cold Bundles

No bundle changes detected.

Cached Bundles

No bundle changes detected.

Three.js ✅

Timings

Description Time Difference
Cold 6.71s +83.00ms
Cached 260.00ms -6.00ms

Cold Bundles

No bundle changes detected.

Cached Bundles

No bundle changes detected.

Click here to view a detailed benchmark overview.

@devongovett devongovett merged commit fdae6c0 into v2 Jan 7, 2023
@devongovett devongovett deleted the fix-circular-hmr branch January 7, 2023 17:20
marcins pushed a commit to marcins/parcel that referenced this pull request Jul 14, 2023
* upstream/v2: (33 commits)
  v2.8.3
  Changelog for v2.8.3
  Address bug by updating an asset reference and merge conditions (parcel-bundler#8762)
  Fix CSS order when merging type change bundles (parcel-bundler#8766)
  fixing failing build for contributors on Linux using Node 18 (parcel-bundler#8763)
  Extension: Importers View and separate LSP protocol package (parcel-bundler#8747)
  Bump swc to fix sourcemaps with Windows line endings (parcel-bundler#8756)
  Apply HMR updates in topological order (parcel-bundler#8752)
  Make extension packaging work (parcel-bundler#8730)
  Typed api.storeResult (parcel-bundler#8732)
  Refactor LSP to use vscode-jsonrpc (parcel-bundler#8728)
  Bump swc (parcel-bundler#8742)
  Recursively check reachability when removing asset graphs from bundles in deduplication (parcel-bundler#6004)
  Fix tsc sourcemaps metadata (parcel-bundler#8734)
  Assigning to `this` in CommonJS (parcel-bundler#8737)
  Don't retarget dependencies if a symbol is imported multiple times with different local names (parcel-bundler#8738)
  Add a note about using flow in CONTRIBUTING.md (parcel-bundler#8731)
  filter out title execArgv to workers (parcel-bundler#8719)
  Document more of the BundleGraph class (parcel-bundler#8711)
  Fixed the hmr connection with host 0.0.0.0 (parcel-bundler#7357)
  ...
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.

3 participants