fix: use correct Cell for EsmLibraryPlugin#12067
Conversation
✅ Deploy Preview for rspack canceled.
|
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the EsmLibraryPlugin to replace OnceCell with AtomicRefCell for two fields: concatenated_modules_map_for_codegen and links. This change allows for more flexible interior mutability patterns and simplifies the code by replacing .get().expect() and .set().expect() calls with standard borrow operations.
Key Changes:
- Replace
OnceCellwithAtomicRefCellfor internal state management - Simplify access patterns using
borrow()andborrow_mut()instead ofget()andset() - Add
atomic_refcellas a new dependency
Reviewed Changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| crates/rspack_plugin_esm_library/Cargo.toml | Adds atomic_refcell dependency |
| Cargo.lock | Updates lock file with new dependency |
| crates/rspack_plugin_esm_library/src/plugin.rs | Replaces OnceCell with AtomicRefCell for two fields and updates access patterns |
| crates/rspack_plugin_esm_library/src/link.rs | Updates mutation logic to use borrow_mut() |
| crates/rspack_plugin_esm_library/src/render.rs | Updates read access to use borrow() and direct indexing |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
📦 Binary Size-limit
❌ Size increased by 10.25KB from 47.85MB to 47.86MB (⬆️0.02%) |
CodSpeed Performance ReportMerging #12067 will not alter performanceComparing Summary
|
16a918b to
5b2b23f
Compare
5b2b23f to
34dcef6
Compare
Summary
Plugin needs to run without any mutable reference, however we do need to store some data in plugin.
EsmLibraryPlugin has some internal state that needs to be initialized in
makephase, and consumed bysealphase, we use OnceCell before, which will panic if user rebuild again.The case is a common write then read case. And because we need to support async, so use AtomicRefCell for such cases.
Related links
Checklist