[WIP] feat: ensure esm imports exists when mode is production #1709
Conversation
Walkthrough本次更改引入了一个新的函数 Changes
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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 (
|
WalkthroughThis pull request introduces a new feature that ensures ECMAScript Module (ESM) imports exist when the mode is set to production. It adds a Changes
|
![petercat-assistant[bot]](https://avatars.githubusercontent.com/in/875736?s=60&v=4){kind=small}
| }); | ||
| }); | ||
| if should_panic { | ||
| panic!("dependency check error!"); |
There was a problem hiding this comment.
Using panic! for dependency check errors in production mode can cause the entire application to crash. Consider using a more graceful error handling mechanism, such as logging the error and continuing execution, or returning a Result with an error variant.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Outside diff range and nitpick comments (18)
crates/mako/src/plugins/dependency_checker/collect_imports.rs (1)
1-8: 结构定义看起来不错，建议添加文档注释结构体的设计很合理，使用了恰当的生命周期标注和集合类型。不过建议为结构体和字段添加文档注释，以便更好地理解其用途。
+/// 收集模块中的导入声明和导出声明 pub struct CollectImports<'a> { + /// 存储导入源与其对应的导入说明符集合的映射关系 pub imports_specifiers_with_source: &'a mut HashMap<String, HashSet<String>>, }crates/mako/src/ast/utils.rs (1)
154-171: 建议优化模块系统检测逻辑函数实现基本正确，但有以下几点建议：
Script分支中的 ESM 检测逻辑与文件中已有的is_esm函数重复，建议复用该函数- 可以考虑为
Custom类型添加注释说明选择该类型的原因建议按如下方式重构：
pub fn get_module_system(ast: &ModuleAst) -> ModuleSystem { match ast { ModuleAst::Script(module) => { - let is_esm = module - .ast - .body - .iter() - .any(|s| matches!(s, ModuleItem::ModuleDecl(_))); - if is_esm { + if is_esm(&module.ast) { ModuleSystem::ESModule } else { ModuleSystem::CommonJS } } + // CSS 和空模块使用自定义模块系统处理 crate::module::ModuleAst::Css(_) => ModuleSystem::Custom, crate::module::ModuleAst::None => ModuleSystem::Custom, } }packages/mako/src/binding.d.ts (1)
8-8: 新增的生命周期钩子需要补充使用文档这些新增的钩子函数扩展了插件系统的能力：
enforce用于控制插件执行顺序writeBundle和buildEnd用于构建完成后的处理watchChanges用于文件变更监听transform和transformInclude用于内容转换建议：
- 为每个钩子添加 JSDoc 注释，说明其用途、执行时机和使用示例
- 在 README 中补充这些新钩子的使用文档
建议添加类似以下的 JSDoc 注释：
+ /** + * 控制插件的执行顺序 + * @example + * enforce: 'pre' // 前置执行 + * enforce: 'post' // 后置执行 + */ enforce?: string;Also applies to: 54-58, 61-61, 67-71
crates/mako/src/build.rs (2)
212-212: 建议增强错误模块的模块系统处理错误模块是合成模块，建议考虑以下改进：
- 为错误模块设置一个固定的模块系统类型，避免不必要的 AST 分析
- 添加错误处理，以防
get_module_system分析失败let info = ModuleInfo { file, - module_system: get_module_system(&ast), + module_system: ModuleSystem::CommonJS, // 错误模块使用固定的模块系统类型 ast, raw, ..Default::default() };
318-318: 建议完善模块系统检测的架构设计在主要的模块构建过程中，模块系统的检测至关重要。建议考虑以下架构改进：
- 添加模块系统检测的缓存机制，避免重复分析
- 考虑在 watch 模式下的增量更新策略
- 为不同的模块系统类型添加特定的构建优化
另外，建议在
ModuleInfo结构体中添加相关文档，说明模块系统检测的重要性和使用注意事项。crates/mako/src/module.rs (1)
36-41: 建议为枚举添加文档注释枚举定义清晰且合理。建议添加文档注释来说明每个变体的具体用途，特别是
Custom变体的使用场景。建议添加如下文档注释：
+/// 表示模块系统的类型 +/// - CommonJS: 使用 require/exports 的 CommonJS 模块 +/// - ESModule: 使用 import/export 的 ES 模块 +/// - Custom: 自定义模块系统实现 #[derive(Debug, Clone, PartialEq, Eq)] pub enum ModuleSystem { CommonJS, ESModule, Custom, }crates/mako/src/plugins/tree_shaking/module.rs (2)
Line range hint
281-297: 建议重构模块系统初始化逻辑建议将以下几点改进：
- 将 ESM 检测逻辑抽取为独立函数，提高代码可维护性
- 统一
unresolved_ctxt的设置逻辑，确保一致性处理+ fn is_esm_module(module: &SwcModule) -> bool { + module.body.iter().any(|s| matches!(s, ModuleItem::ModuleDecl(_))) + } let module_system = module_info.module_system.clone(); let stmt_graph = match &module_info.ast { crate::module::ModuleAst::Script(module) => { - let is_esm = module - .ast - .body - .iter() - .any(|s| matches!(s, ModuleItem::ModuleDecl(_))); + let is_esm = is_esm_module(&module.ast); if is_esm { unresolved_ctxt = unresolved_ctxt.apply_mark(module.unresolved_mark); StatementGraph::new(&module.ast, unresolved_ctxt) } else { StatementGraph::empty() } }
Line range hint
314-322: 建议添加模块系统相关的副作用处理文档当前代码根据模块系统类型（ESModule/CommonJS/Custom）来确定副作用和导出行为，建议添加注释说明以下几点：
- 为什么 ESModule 默认无副作用
- 各种模块系统下 AllExports 初始化的差异原因
- 副作用判断逻辑的设计考虑
side_effects: module_system != ModuleSystem::ESModule, side_effect_dep_sources: Default::default(), is_async: module.info.as_ref().unwrap().is_async, +// ESModule 默认无副作用，因为其具有静态导入导出特性 +// CommonJS 和 Custom 模块系统由于其动态特性，默认视为有副作用 all_exports: match module_system { ModuleSystem::ESModule => AllExports::Precise(Default::default()), ModuleSystem::Custom | ModuleSystem::CommonJS => { AllExports::Ambiguous(Default::default()) } },crates/mako/src/plugins/dependency_checker/collect_exports.rs (2)
57-57: 请完成ExportDefaultSpecifier的实现代码中存在
//@todo注释，表示尚未处理ExportDefaultSpecifier。为确保导出收集的完整性，请实现对该情况的处理。如果需要，我可以协助提供此部分的实现，您希望我为此创建一个新的 GitHub issue 吗？
31-35: 建议使用常量替代硬编码字符串在多处直接使用了
"default".to_string()，建议将"default"定义为常量，避免魔法字符串，增强代码可维护性。可以在代码顶部添加：
const DEFAULT_EXPORT: &str = "default";并修改相关代码为：
- self.specifiers.remove(&"default".to_string()); + self.specifiers.remove(&DEFAULT_EXPORT.to_string());crates/mako/src/plugins/dependency_checker.rs (3)
28-51: 完善匹配分支，处理所有可能的ModuleSystem类型在匹配
info.module_system时，只处理了ESModule、CommonJS和Custom三种类型。若未来增加新的模块系统类型，可能会导致未匹配的情况。建议添加默认分支，确保所有可能的枚举值都被处理。修改建议：
match info.module_system { ModuleSystem::ESModule => { // existing code } ModuleSystem::CommonJS | ModuleSystem::Custom => { specifiers.clear(); + } + _ => { + // 处理其他未匹配的模块系统类型 + specifiers.clear(); } }
67-70: 简化条件判断，提高代码可读性在第68至70行的条件判断中，嵌套了多个条件，可以通过使用守卫条件来简化，提高代码的可读性。
修改建议：
- if let Some(info) = &m.info { - if !info.file.is_under_node_modules - && matches!(info.module_system, ModuleSystem::ESModule) - { + if let Some(info) = &m.info + if !info.file.is_under_node_modules + && matches!(info.module_system, ModuleSystem::ESModule) { // 收集 imports
108-115: 避免多次赋值，直接返回结果在第108至115行，
should_panic被多次赋值为true，可以在确认有未定义的导入规范符时，直接返回错误或收集所有错误后统一处理，避免重复赋值。修改建议：
- .for_each(|(source, specifiers)| { - should_panic = true; + .filter(|(_, specifiers)| !specifiers.is_empty()) + .flat_map(|(source, specifiers)| specifiers.iter().map(move |specifier| (source, specifier))) + .for_each(|(source, specifier)| { + error!( + "'{}' is undefined: import from '{}' in '{}'", + specifier, source, module_id.id + ); }); - }); -}); -if should_panic { - return Err(anyhow::anyhow!("dependency check error!")); -};crates/mako/src/compiler.rs (1)
261-263: 建议在开发模式下也启用 DependencyChecker 插件依赖检查在开发阶段同样重要，可以及早发现潜在问题，避免问题延续到生产环境。建议在开发模式下也添加
DependencyChecker插件，或者提供配置选项供用户选择是否启用。crates/mako/src/plugins/tree_shaking/shake/module_concatenate.rs (1)
26-26: 确保移除不必要的导入以提高代码清晰度在第26行，您从
crate::plugins::tree_shaking::module中不再导入ModuleSystem：use crate::plugins::tree_shaking::module::{AllExports, TreeShakeModule};如果
ModuleSystem已被正确地从其他路径导入，移除冗余的导入有助于提高代码的可读性和维护性。crates/mako/src/plugins/tree_shaking/shake.rs (3)
Line range hint
238-240: 潜在的逻辑问题：collect_all_exports_of函数中的递归循环在
collect_all_exports_of函数中，先获取了tsm的引用，然后检查是否已访问过当前module_id。如果该模块已访问过，函数会返回，但在返回前并未释放tsm的引用，可能导致重复的不可变借用，造成运行时错误。建议在获取
tsm引用之前，先检查并更新visited集合，以避免潜在的借用冲突。fn collect_all_exports_of( module_id: &ModuleId, tree_shake_modules_map: &HashMap<ModuleId, RefCell<TreeShakeModule>>, module_graph: &ModuleGraph, all_exports: &mut AllExports, visited: &mut HashSet<ModuleId>, ) { + if visited.contains(module_id) { + return; + } + visited.insert(module_id.clone()); let tsm = tree_shake_modules_map.get(module_id).unwrap().borrow(); - - if visited.contains(module_id) { - return; - } - - visited.insert(module_id.clone());
Line range hint
245-250: 性能建议：减少对strip_context函数的重复调用在处理导出和导入的标识符时，多次调用了
strip_context函数，可能导致不必要的性能开销。建议缓存处理结果或优化调用方式，以提高代码效率。
Line range hint
360-368: 代码优化：简化strip_context函数的实现
strip_context函数当前使用了split和collect方法，可以通过使用split_once方法简化实现，使代码更简洁明了。pub fn strip_context(ident: &str) -> String { - let ident_split = ident.split('#').collect::<Vec<_>>(); - ident_split[0].to_string() + ident.split_once('#') + .map_or_else(|| ident.to_string(), |(first, _)| first.to_string()) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (12)
crates/mako/src/ast/utils.rs(2 hunks)crates/mako/src/build.rs(4 hunks)crates/mako/src/compiler.rs(2 hunks)crates/mako/src/module.rs(2 hunks)crates/mako/src/plugins.rs(1 hunks)crates/mako/src/plugins/dependency_checker.rs(1 hunks)crates/mako/src/plugins/dependency_checker/collect_exports.rs(1 hunks)crates/mako/src/plugins/dependency_checker/collect_imports.rs(1 hunks)crates/mako/src/plugins/tree_shaking/module.rs(3 hunks)crates/mako/src/plugins/tree_shaking/shake.rs(1 hunks)crates/mako/src/plugins/tree_shaking/shake/module_concatenate.rs(1 hunks)packages/mako/src/binding.d.ts(4 hunks)
🔇 Additional comments (13)
crates/mako/src/plugins.rs (2)
6-6: 新模块声明看起来没有问题！
新增的 dependency_checker 模块声明与其他插件模块保持一致的可见性和命名风格。
6-6: 建议验证新模块的实现
由于这是一个新增的依赖检查器模块，需要确保其实现符合预期功能。
运行以下脚本来检查新模块的实现：
✅ Verification successful
依赖检查器模块实现完整且功能明确
经过验证，dependency_checker 模块实现了以下功能：
- 正确实现了 Plugin trait，并在 after_build 钩子中执行依赖检查
- 完整处理了 ESM 模块系统的导入导出逻辑：
- 收集并验证所有 import 声明的有效性
- 检查导入的标识符是否在依赖模块中有对应的导出
- 对未定义的导入会给出明确的错误提示
模块的实现符合预期的依赖检查功能要求。
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# 描述：验证 dependency_checker 模块的实现
# 检查模块文件是否存在
fd -t f "dependency_checker.rs" "crates/mako/src"
# 检查模块中是否实现了 Plugin trait
ast-grep --pattern 'impl Plugin for DependencyChecker'
# 检查是否包含 ESM 相关的导入检查逻辑
rg -A 5 "esm|import|export" "crates/mako/src/plugins/dependency_checker"
Length of output: 15229
crates/mako/src/ast/utils.rs (1)
7-7: 导入声明看起来没有问题！
新增的导入语句清晰地引入了所需的类型，并且与现有代码风格保持一致。
packages/mako/src/binding.d.ts (1)
168-168: 配置项需要补充说明文档
新增的 inlineExcludesExtensions 配置项：
- 需要补充说明其用途和配置示例
- 建议验证此配置项是否与 PR 目标（确保生产环境下的 ESM 导入）相关
crates/mako/src/build.rs (2)
16-16: 导入声明看起来没有问题！
新增的 get_module_system 导入与其他 AST 相关的导入保持一致，并且在整个文件中被合理使用。
187-187: 建议验证外部模块的模块系统检测
对于外部模块，正确识别其模块系统类型（CommonJS/ESM）非常重要，特别是在生产环境中。建议验证以下几点：
- 外部模块的 AST 解析是否正确
get_module_system是否能正确处理外部模块的特殊情况
✅ Verification successful
Let me gather more information about the module system detection implementation.
Based on the results, I can now provide a final response as we have found the implementation of get_module_system and its usage.
外部模块的模块系统检测已得到适当处理
get_module_system 函数实现了可靠的模块系统检测机制：
- 通过检查 AST 中是否存在
ModuleDecl节点来判断是否为 ESM - 该实现同时适用于外部模块和内部模块
- 在
create_external_module中正确集成了模块系统检测
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# 搜索外部模块相关的测试用例
rg -l "create_external_module|external.*module" --type rust
# 检查 get_module_system 的实现细节
ast-grep --pattern 'fn get_module_system($_) {
$$$
}'
Length of output: 774
Script:
#!/bin/bash
# 搜索 get_module_system 函数的定义和使用
rg -A 10 "get_module_system" --type rust
# 查找外部模块相关的测试代码
rg -A 5 "create_external_module.*test" --type rust
# 检查是否有专门针对模块系统的测试
rg -l "module.*system.*test" --type rust
Length of output: 3344
crates/mako/src/module.rs (2)
208-208: 验证默认值对现有代码的影响
将 module_system 默认值设置为 CommonJS 可能会影响现有的模块处理逻辑。建议验证这个默认值是否与现有代码的期望行为一致。
运行以下脚本来验证潜在影响：
✅ Verification successful
Based on the search results, I can now generate the final response since I have enough context to verify the impact of setting ModuleSystem::CommonJS as the default value.
默认使用 CommonJS 模块系统是合理的选择
代码分析显示这个默认值设置是安全的，原因如下：
- 代码中已经完整处理了 CommonJS 和 ESM 两种模块系统的场景
- 模块系统类型会根据源文件内容自动检测（通过
is_esm函数），不依赖默认值 - 即使默认设为 CommonJS，ESM 模块的处理逻辑仍然会正确执行
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# 描述：检查可能受默认 CommonJS 设置影响的代码
# 搜索涉及模块系统判断的代码
rg "(?i)(esmodule|commonjs|cjs|esm)" -t rust
# 搜索模块初始化相关的代码
ast-grep --pattern 'ModuleInfo::default()'
Length of output: 22646
202-202: 验证 module_system 字段的使用情况
新增的 module_system 字段需要确保在整个代码库中得到正确使用。
运行以下脚本来验证字段的使用情况：
✅ Verification successful
Let me gather more information about the get_module_system function implementation to ensure it's properly handling the new field.
Let me check one more thing to see how the module system is being determined and used in the codebase.
module_system 字段的使用已得到完整验证
根据代码分析，module_system 字段在整个代码库中得到了正确且一致的使用：
- 在
ast::utils::get_module_system中正确判断模块类型 - 在依赖检查器（dependency_checker）中用于区分处理 ESM 和 CommonJS 模块
- 在 tree-shaking 相关代码中用于优化处理
- 默认值设置为
ModuleSystem::CommonJS
所有使用场景都符合预期，没有发现任何问题。
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# 描述：检查 module_system 字段在代码库中的使用情况
# 搜索所有对 module_system 的引用
rg "module_system" -t rust
# 搜索可能需要更新的模块系统判断逻辑
ast-grep --pattern 'if $expr.is_esm() { $$$ }'
Length of output: 1632
Script:
#!/bin/bash
# 查看 get_module_system 函数的实现
ast-grep --pattern 'fn get_module_system($$$) { $$$ }'
# 检查 ModuleSystem 枚举的定义
rg "enum ModuleSystem" -A 5 -t rust
Length of output: 317
Script:
#!/bin/bash
# 搜索 ModuleSystem 的使用上下文
rg "ModuleSystem::" -B 2 -A 2 -t rust
# 查找可能的模块系统相关配置
rg "module.*system" -i -t rust
Length of output: 7699
crates/mako/src/compiler.rs (2)
18-18: 引入新的 Mode 枚举
成功引入了 Mode 枚举，为配置添加模式支持，便于根据不同模式进行条件处理。
261-263: 在生产模式下添加 DependencyChecker 插件
当 config.mode 为 Mode::Production 时，添加了 DependencyChecker 插件，以增强对依赖项的检查，确保生产环境的稳定性。
crates/mako/src/plugins/tree_shaking/shake/module_concatenate.rs (1)
24-24: 确认 ModuleSystem 导入路径的正确性
在第24行，您从 crate::module 导入了 ModuleSystem：
use crate::module::{Dependency, ImportType, ModuleId, ModuleSystem, ResolveType};同时，第26行移除了从 crate::plugins::tree_shaking::module 导入的 ModuleSystem。请确保在本文件中使用的 ModuleSystem 类型确实应从 crate::module 导入，以避免潜在的命名冲突或类型不匹配。
您可以运行以下脚本来检查 ModuleSystem 的定义及其在代码库中的使用情况：
✅ Verification successful
Based on the results, I can now generate the final response since I have clear evidence that ModuleSystem is correctly defined in crate::module and is being used consistently across the codebase.
导入路径验证已完成，无需更改
ModuleSystem 的导入路径是正确的。通过代码分析可以确认：
ModuleSystem枚举类型确实定义在crate::module模块中- 整个代码库中的其他文件也都是从
crate::module导入ModuleSystem - 移除
crate::plugins::tree_shaking::module的导入是正确的，因为该模块只是使用而非定义ModuleSystem
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# 描述：检查 `ModuleSystem` 的定义位置和引用情况
# 查找 `ModuleSystem` 的定义位置
rg -A 5 "enum ModuleSystem" crates/mako/src/
# 查找引用了 `ModuleSystem` 的文件
rg "ModuleSystem" crates/mako/src/ -g '!*/module.rs'
Length of output: 2579
crates/mako/src/plugins/tree_shaking/shake.rs (2)
18-18: 建议：全面处理新增的 ModuleSystem 类型
第18行新增了对 ModuleSystem 的引用。请确保在代码中所有涉及模块系统的逻辑都正确地处理了 ModuleSystem 的所有可能变体，避免遗漏对某些模块系统类型的处理，确保模块处理的完整性和正确性。
Line range hint 83-93: 可能的逻辑错误：greater_equal_than 函数的比较逻辑
在 greater_equal_than 函数中，当参数 b < 0 时，函数返回 true。但是在调用该函数的上下文中，可能需要仔细检查这种情况下返回 true 是否符合预期，以避免潜在的逻辑错误。
请验证函数的实现是否满足预期的逻辑需求，必要时进行调整。
| fn visit_import_decl(&mut self, node: &ImportDecl) { | ||
| let source = node.src.value.to_string(); | ||
| if self.imports_specifiers_with_source.get(&source).is_none() { | ||
| self.imports_specifiers_with_source | ||
| .insert(source.clone(), HashSet::new()); | ||
| } | ||
|
|
||
| node.specifiers | ||
| .iter() | ||
| .for_each(|specifier| match specifier { | ||
| ImportSpecifier::Named(named) => { | ||
| if let Some(ModuleExportName::Ident(ident)) = &named.imported { | ||
| self.imports_specifiers_with_source | ||
| .get_mut(&source) | ||
| .unwrap() | ||
| .insert(ident.sym.to_string()); | ||
| } else { | ||
| self.imports_specifiers_with_source | ||
| .get_mut(&source) | ||
| .unwrap() | ||
| .insert(named.local.sym.to_string()); | ||
| } | ||
| } | ||
| ImportSpecifier::Default(_) => { | ||
| self.imports_specifiers_with_source | ||
| .get_mut(&source) | ||
| .unwrap() | ||
| .insert("default".into()); | ||
| } | ||
| _ => {} | ||
| }) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
建议改进错误处理机制
当前实现中多次使用了 unwrap()，这在生产环境中可能不够安全。建议使用 expect() 提供更明确的错误信息，或者使用 unwrap_or_default() 来优雅处理错误情况。
- .unwrap()
+ .expect("Import source should exist in map")另外，建议添加对 ImportSpecifier::Namespace 的处理，以支持 import * as name 语法。
Committable suggestion skipped: line range outside the PR's diff.
| fn visit_named_export(&mut self, node: &NamedExport) { | ||
| let source = node.src.clone().unwrap().value; | ||
| if self | ||
| .imports_specifiers_with_source | ||
| .get(source.as_str()) | ||
| .is_none() | ||
| { | ||
| self.imports_specifiers_with_source | ||
| .insert(source.to_string(), HashSet::new()); | ||
| } | ||
|
|
||
| if node.src.is_some() { | ||
| node.specifiers.iter().for_each(|specifier| { | ||
| if let ExportSpecifier::Named(named) = specifier { | ||
| if let ModuleExportName::Ident(ident) = &named.orig { | ||
| self.imports_specifiers_with_source | ||
| .get_mut(source.as_str()) | ||
| .unwrap() | ||
| .insert(ident.sym.to_string()); | ||
| } | ||
| } | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
存在潜在的运行时错误风险
在处理命名导出时存在以下问题：
- 第45行直接解包
node.src，但实际上导出声明的src可能为None - 第55行的检查与之前的解包操作矛盾
建议修改如下：
- let source = node.src.clone().unwrap().value;
- if self
- .imports_specifiers_with_source
- .get(source.as_str())
- .is_none()
- {
- self.imports_specifiers_with_source
- .insert(source.to_string(), HashSet::new());
- }
-
- if node.src.is_some() {
+ if let Some(src) = &node.src {
+ let source = src.value.to_string();
+ self.imports_specifiers_with_source
+ .entry(source.clone())
+ .or_default();
+
node.specifiers.iter().for_each(|specifier| {
if let ExportSpecifier::Named(named) = specifier {
if let ModuleExportName::Ident(ident) = &named.orig {
self.imports_specifiers_with_source
- .get_mut(source.as_str())
- .unwrap()
+ .get_mut(&source)
+ .expect("Source should exist in map")
.insert(ident.sym.to_string());
}
}
})
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| fn visit_named_export(&mut self, node: &NamedExport) { | |
| let source = node.src.clone().unwrap().value; | |
| if self | |
| .imports_specifiers_with_source | |
| .get(source.as_str()) | |
| .is_none() | |
| { | |
| self.imports_specifiers_with_source | |
| .insert(source.to_string(), HashSet::new()); | |
| } | |
| if node.src.is_some() { | |
| node.specifiers.iter().for_each(|specifier| { | |
| if let ExportSpecifier::Named(named) = specifier { | |
| if let ModuleExportName::Ident(ident) = &named.orig { | |
| self.imports_specifiers_with_source | |
| .get_mut(source.as_str()) | |
| .unwrap() | |
| .insert(ident.sym.to_string()); | |
| } | |
| } | |
| }) | |
| } | |
| } | |
| fn visit_named_export(&mut self, node: &NamedExport) { | |
| if let Some(src) = &node.src { | |
| let source = src.value.to_string(); | |
| self.imports_specifiers_with_source | |
| .entry(source.clone()) | |
| .or_default(); | |
| node.specifiers.iter().for_each(|specifier| { | |
| if let ExportSpecifier::Named(named) = specifier { | |
| if let ModuleExportName::Ident(ident) = &named.orig { | |
| self.imports_specifiers_with_source | |
| .get_mut(&source) | |
| .expect("Source should exist in map") | |
| .insert(ident.sym.to_string()); | |
| } | |
| } | |
| }) | |
| } | |
| } |
| export interface WatchChangesParams { | ||
| event: string; | ||
| } |
There was a problem hiding this comment.
接口类型定义存在不一致
发现以下问题：
WatchChangesParams中的event类型与watchChanges钩子中使用的字面量类型不匹配TransformResult中的type属性应该限制为'css' | 'js'以保持类型安全
建议修改如下：
export interface WatchChangesParams {
- event: string;
+ event: 'create' | 'delete' | 'update';
}
export interface TransformResult {
content: string;
- type: string;
+ type: 'css' | 'js';
}Also applies to: 91-94
| Decl::Var(box VarDecl { decls, .. }) => decls.iter().for_each(|decl| { | ||
| if let Pat::Ident(ident) = &decl.name { | ||
| self.specifiers.remove(&ident.sym.to_string()); | ||
| } | ||
| }), |
There was a problem hiding this comment.
必须处理其他变量声明模式以确保完整性
当前代码在处理变量声明时仅考虑了 Pat::Ident 模式，未处理解构赋值等其他模式。这可能导致某些导出变量未被正确移除，影响功能正确性。
建议修改代码以处理更多的变量模式，示例如下：
- if let Pat::Ident(ident) = &decl.name {
- self.specifiers.remove(&ident.sym.to_string());
- }
+ match &decl.name {
+ Pat::Ident(ident) => {
+ self.specifiers.remove(&ident.sym.to_string());
+ }
+ Pat::Array(array_pat) => {
+ // 处理数组解构模式
+ for element in &array_pat.elems {
+ if let Some(Pat::Ident(ident)) = element {
+ self.specifiers.remove(&ident.sym.to_string());
+ }
+ // 递归处理嵌套模式
+ }
+ }
+ Pat::Object(object_pat) => {
+ // 处理对象解构模式
+ for prop in &object_pat.props {
+ match prop {
+ ObjectPatProp::KeyValue(kv) => {
+ if let Pat::Ident(ident) = &kv.value {
+ self.specifiers.remove(&ident.sym.to_string());
+ }
+ }
+ ObjectPatProp::Assign(assign) => {
+ self.specifiers.remove(&assign.key.sym.to_string());
+ }
+ _ => {}
+ }
+ // 递归处理嵌套模式
+ }
+ }
+ _ => {}
+ }Committable suggestion skipped: line range outside the PR's diff.
| named.specifiers.iter().for_each(|specifier| { | ||
| match &specifier { | ||
| ExportSpecifier::Named(named) => { | ||
| if let Some(ModuleExportName::Ident(ident)) = &named.exported { | ||
| self.specifiers.remove(&ident.sym.to_string()); | ||
| } | ||
| } | ||
| ExportSpecifier::Namespace(name_spacing) => { | ||
| if let ModuleExportName::Ident(ident) = &name_spacing.name { | ||
| self.specifiers.remove(&ident.sym.to_string()); | ||
| } | ||
| } | ||
| _ => { | ||
| //@todo what is ExportDefaultSpecifier? | ||
| } | ||
| } | ||
| }) |
There was a problem hiding this comment.
需要处理 ExportDefaultSpecifier 以完整支持所有导出情况
当前匹配逻辑未处理 ExportSpecifier::Default，这可能导致默认导出未被正确识别和处理，可能遗漏相关的导出信息。
建议在匹配语句中添加对 ExportSpecifier::Default 的处理，例如：
match &specifier {
ExportSpecifier::Named(named) => {
// 原有处理逻辑
}
ExportSpecifier::Namespace(namespace) => {
// 原有处理逻辑
}
+ ExportSpecifier::Default(default_specifier) => {
+ self.specifiers.remove(&default_specifier.exported.sym.to_string());
+ }
_ => {}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| named.specifiers.iter().for_each(|specifier| { | |
| match &specifier { | |
| ExportSpecifier::Named(named) => { | |
| if let Some(ModuleExportName::Ident(ident)) = &named.exported { | |
| self.specifiers.remove(&ident.sym.to_string()); | |
| } | |
| } | |
| ExportSpecifier::Namespace(name_spacing) => { | |
| if let ModuleExportName::Ident(ident) = &name_spacing.name { | |
| self.specifiers.remove(&ident.sym.to_string()); | |
| } | |
| } | |
| _ => { | |
| //@todo what is ExportDefaultSpecifier? | |
| } | |
| } | |
| }) | |
| named.specifiers.iter().for_each(|specifier| { | |
| match &specifier { | |
| ExportSpecifier::Named(named) => { | |
| if let Some(ModuleExportName::Ident(ident)) = &named.exported { | |
| self.specifiers.remove(&ident.sym.to_string()); | |
| } | |
| } | |
| ExportSpecifier::Namespace(name_spacing) => { | |
| if let ModuleExportName::Ident(ident) = &name_spacing.name { | |
| self.specifiers.remove(&ident.sym.to_string()); | |
| } | |
| } | |
| ExportSpecifier::Default(default_specifier) => { | |
| self.specifiers.remove(&default_specifier.exported.sym.to_string()); | |
| } | |
| _ => {} | |
| } | |
| }) |
| specifiers: &mut HashSet<String>, | ||
| ) { | ||
| if !specifiers.is_empty() { | ||
| let dep_module = module_graph.get_module(module_id).unwrap(); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
建议避免使用 unwrap()，采用安全的错误处理
在第26行，使用了 module_graph.get_module(module_id).unwrap()，若模块不存在，将导致程序发生 panic。为了提高代码的健壮性，建议匹配返回值，妥善处理 None 情况。
可以参考以下修改：
-let dep_module = module_graph.get_module(module_id).unwrap();
+let dep_module = match module_graph.get_module(module_id) {
+ Some(module) => module,
+ None => {
+ // 处理模块不存在的情况，例如记录错误日志或返回错误
+ return;
+ }
+};Committable suggestion skipped: line range outside the PR's diff.
| if should_panic { | ||
| panic!("dependency check error!"); | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
避免使用 panic!，改为返回错误以提供更好的错误处理
在第118行，直接调用 panic! 会导致程序崩溃，不利于错误的追踪和处理。建议返回 Err，让调用者决定如何处理错误，增强代码的鲁棒性。
可以参考以下修改：
if should_panic {
- panic!("dependency check error!");
+ return Err(anyhow::anyhow!("dependency check error!"));
};Committable suggestion skipped: line range outside the PR's diff.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1709 +/- ##
==========================================
+ Coverage 55.28% 55.31% +0.03%
==========================================
Files 175 178 +3
Lines 17696 17835 +139
==========================================
+ Hits 9783 9866 +83
- Misses 7913 7969 +56 ☔ View full report in Codecov by Sentry. |
why not dev warning ?
will speed down the hmr build
Summary by CodeRabbit
新功能
ModuleSystem枚举，支持CommonJS、ESModule和Custom模块类型。DependencyChecker插件，增强模块依赖分析能力。JsHooks接口，增加了多个新方法和属性以支持内容转换和变更监控。修复