fix(hmr): preserve original import.meta.hot when no HMR context exists#4391
fix(hmr): preserve original import.meta.hot when no HMR context exists#4391underfin merged 2 commits intorolldown:mainfrom
import.meta.hot when no HMR context exists#4391Conversation
WalkthroughThe changes update the internal logic of the Changes
Sequence Diagram(s)sequenceDiagram
participant TestRunner
participant ScopeHoistingFinalizer
participant JSModule
TestRunner->>JSModule: Execute main.js (logs import.meta.hot)
JSModule->>ScopeHoistingFinalizer: Encounter import.meta.hot
ScopeHoistingFinalizer->>ScopeHoistingFinalizer: Check hmr_hot_ref (may be None)
alt hmr_hot_ref is Some
ScopeHoistingFinalizer->>JSModule: Rewrite import.meta.hot
else hmr_hot_ref is None
ScopeHoistingFinalizer->>JSModule: Leave import.meta.hot unchanged
end
JSModule->>TestRunner: Output result (no crash)
Assessment against linked issues
Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
crates/rolldown/src/module_finalizers/scope_hoisting/hmr.rs (1)
125-136: 💡 Verification agent🧩 Analysis chain
Potential consistency issue with hmr_hot_ref handling
This method still unconditionally uses
unwrap()onhmr_hot_refwhich could cause panics in similar scenarios. Consider applying the same pattern used inrewrite_import_meta_hotfor consistency and to prevent potential issues.
🏁 Script executed:
#!/bin/bash # Verify if generate_stmt_of_init_module_hot_context is only called when hmr_hot_ref is Some rg -A 3 -B 3 "generate_stmt_of_init_module_hot_context"Length of output: 2686
🏁 Script executed:
#!/bin/bash sed -n '1,200p' crates/rolldown/src/module_finalizers/scope_hoisting/hmr.rsLength of output: 6596
🏁 Script executed:
#!/bin/bash rg -n "struct ScopeHoistingFinalizer" -C 5 crates/rolldown/src/module_finalizers/scope_hoistingLength of output: 2279
🏁 Script executed:
#!/bin/bash sed -n '1,200p' crates/rolldown/src/hmr/hmr_ast_finalizer.rsLength of output: 7201
Guard unwrap on
hmr_hot_refin HMR scope-hoisting finalizerThe method
generate_stmt_of_init_module_hot_contextin
crates/rolldown/src/module_finalizers/scope_hoisting/hmr.rsunconditionally callsself.ctx.module.ecma_view.hmr_hot_ref.unwrap()even though
generate_hmr_headeronly checksoptions.is_hmr_enabled()and not the presence of a hot-ref. Ifhmr_hot_refisNone, this will panic at runtime.• File:
crates/rolldown/src/module_finalizers/scope_hoisting/hmr.rs
Location:generate_stmt_of_init_module_hot_context(around line 125)
• File: same, ingenerate_hmr_header(around line 104) where the call to
generate_stmt_of_init_module_hot_context()livesSuggested fix: only call into or unwrap the hot-ref when it’s actually
Some, mirroringrewrite_import_meta_hot. For example:--- a/crates/rolldown/src/module_finalizers/scope_hoisting/hmr.rs +++ b/crates/rolldown/src/module_finalizers/scope_hoisting/hmr.rs @@ pub fn generate_hmr_header(&self) -> Vec<ast::Statement<'ast>> { - // import.meta.hot = __rolldown_runtime__.createModuleHotContext(moduleId); - ret.push(self.generate_stmt_of_init_module_hot_context()); + // import.meta.hot = __rolldown_runtime__.createModuleHotContext(moduleId); + if let Some(hot_ref) = self.ctx.module.ecma_view.hmr_hot_ref { + ret.push(self.generate_stmt_of_init_module_hot_context(hot_ref)); + }--- a/crates/rolldown/src/module_finalizers/scope_hoisting/hmr.rs +++ b/crates/rolldown/src/module_finalizers/scope_hoisting/hmr.rs @@-125,7 +125,7 @@ impl<'ast> ScopeHoistingFinalizer<'_, 'ast> { pub fn generate_stmt_of_init_module_hot_context(&self) -> ast::Statement<'ast> { - let hot_name = self.canonical_name_for(self.ctx.module.ecma_view.hmr_hot_ref.unwrap()); + pub fn generate_stmt_of_init_module_hot_context(&self, hmr_hot_ref: SymbolId) -> ast::Statement<'ast> { + let hot_name = self.canonical_name_for(hmr_hot_ref);This ensures we only unwrap when a hot context really exists and keeps consistency with
rewrite_import_meta_hot.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
crates/rolldown/tests/rolldown/issues/4390/artifacts.snapis excluded by!**/*.snapcrates/rolldown/tests/snapshots/integration_rolldown__filename_with_hash.snapis excluded by!**/*.snap
📒 Files selected for processing (4)
crates/rolldown/src/module_finalizers/scope_hoisting/hmr.rs(1 hunks)crates/rolldown/tests/rolldown/issues/4390/_config.json(1 hunks)crates/rolldown/tests/rolldown/issues/4390/main.js(1 hunks)crates/rolldown/tests/rolldown/issues/4390/package.json(1 hunks)
🔇 Additional comments (3)
crates/rolldown/tests/rolldown/issues/4390/main.js (1)
1-1: Test looks good for verifying the fixThis test file is a simple but effective verification for the fix. It logs
import.meta.hotto console, which is exactly what's needed to confirm that accessingimport.meta.hotdoesn't cause a runtime panic when no HMR context exists.crates/rolldown/tests/rolldown/issues/4390/_config.json (1)
1-3: Empty config is appropriate for this testThe empty configuration is suitable for this test case since we don't need any special configuration to test the basic behavior of
import.meta.hot. This ensures the test runs with default settings, which is what we want to verify.crates/rolldown/tests/rolldown/issues/4390/package.json (1)
1-3: Minimal package.json is sufficientThe minimal package definition with just a name is appropriate for this test. No additional configuration is needed to reproduce and verify the fix for
import.meta.hothandling.
CodSpeed Performance ReportMerging #4391 will improve performances by 5.38%Comparing Summary
Benchmarks breakdown
|
2 participants
Description
called Option::unwrap() on a None valuewhen input containsimport.meta.hot#4390Summary by CodeRabbit
Bug Fixes
Tests