[WIP] fix: treeshaking & skipmodule should jump the async module #1661
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 (
|
Jinbao1001
changed the title
Jinbao1001
changed the title
Jinbao1001
changed the title
Jinbao1001
changed the title
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1661 +/- ##
==========================================
- Coverage 55.61% 55.56% -0.05%
==========================================
Files 173 173
Lines 17502 17524 +22
==========================================
+ Hits 9733 9737 +4
- Misses 7769 7787 +18 ☔ View full report in Codecov by Sentry. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 16
🧹 Outside diff range and nitpick comments (6)
e2e/fixtures/javascript.async_module_in_loop/async.js (1)
9-11: 导出语句的格式建议为了保持代码的一致性，建议将所有导出合并到一个语句中。
建议修改为：
-export default x; - -export {named} +export { named, default as default }e2e/fixtures/javascript.async_module_in_loop/component.js (1)
5-7: 考虑添加类型注释为了提高代码的可维护性和类型安全性，建议为
config对象添加类型注释。建议添加类型注释：
+/** @type {{key: string}} */ export const config = { key: "value" }e2e/fixtures/javascript.async_module_in_loop/expect.js (1)
6-7: 建议添加错误处理机制当前代码在解析构建结果和初始化测试框架时没有进行错误处理。建议添加 try-catch 块来优雅地处理可能出现的异常。
建议修改如下：
-const { files } = parseBuildResult(__dirname); -injectSimpleJest(); +try { + const { files } = parseBuildResult(__dirname); + injectSimpleJest(); +} catch (error) { + console.error('测试环境初始化失败:', error); + process.exit(1); +}packages/mako/src/binding.d.ts (2)
8-8: 新增的 enforce 属性需要补充文档说明enforce 属性用于控制插件的执行顺序，但缺少对可选值的说明文档。建议添加注释说明其用途和可选值（例如 "pre" 和 "post"）。
+ /** 控制插件执行顺序的属性。可选值: 'pre' | 'post' */ enforce?: string;
168-168: inlineExcludesExtensions 配置项需要补充说明新增的 inlineExcludesExtensions 配置项缺少注释说明其用途和示例。
+ /** + * 指定不进行内联处理的文件扩展名列表 + * @example ['.png', '.jpg'] + */ inlineExcludesExtensions?: string[];crates/mako/src/compiler.rs (1)
36-36: 建议为新字段添加文档注释为了提高代码的可维护性，建议添加文档注释来说明：
- 此字段的用途
- 何时更新此映射
- 何时读取此映射
- 为什么选择 RwLock 作为并发控制机制
建议添加如下文档注释：
+ /// 存储模块的异步依赖关系映射 + /// 在编译期间由异步模块处理器更新 + /// 在生成阶段被转换器读取以处理异步导入 pub async_deps_map: RwLock<HashMap<ModuleId, Vec<Dependency>>>,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (15)
- .gitignore (1 hunks)
- crates/mako/src/compiler.rs (4 hunks)
- crates/mako/src/generate.rs (2 hunks)
- crates/mako/src/generate/transform.rs (3 hunks)
- crates/mako/src/module.rs (1 hunks)
- crates/mako/src/plugins/tree_shaking/shake.rs (1 hunks)
- crates/mako/src/plugins/tree_shaking/shake/skip_module.rs (0 hunks)
- crates/mako/src/visitors/async_module.rs (11 hunks)
- e2e/fixtures/javascript.async_module_in_loop/async.js (1 hunks)
- e2e/fixtures/javascript.async_module_in_loop/component.js (1 hunks)
- e2e/fixtures/javascript.async_module_in_loop/expect.js (1 hunks)
- e2e/fixtures/javascript.async_module_in_loop/index.js (1 hunks)
- e2e/fixtures/javascript.async_module_in_loop/mako.config.json (1 hunks)
- e2e/fixtures/javascript.async_module_in_loop/utils.js (1 hunks)
- packages/mako/src/binding.d.ts (4 hunks)
💤 Files with no reviewable changes (1)
- crates/mako/src/plugins/tree_shaking/shake/skip_module.rs
✅ Files skipped from review due to trivial changes (2)
- .gitignore
- e2e/fixtures/javascript.async_module_in_loop/mako.config.json
🔇 Additional comments (14)
e2e/fixtures/javascript.async_module_in_loop/index.js (1)
1-1: 验证 displayConfig 的异步实现需要确认
displayConfig函数是否正确实现了异步逻辑。执行以下脚本验证实现：
e2e/fixtures/javascript.async_module_in_loop/async.js (1)
1-11: 验证顶层 await 的兼容性此模块使用了顶层 await，需要确保在目标环境中得到支持。
e2e/fixtures/javascript.async_module_in_loop/component.js (1)
9-11:⚠️ Potential issue函数实现与命名不符
displayConfig函数名称暗示它应该展示配置信息，但实际上它只是返回listKeys()的结果，没有使用导出的config对象。这可能会导致使用此函数的开发者产生误解。让我们验证一下
listKeys函数的实现：建议修改函数实现以符合其命名：
export function displayConfig() { - return listKeys() + return listKeys(config) }e2e/fixtures/javascript.async_module_in_loop/expect.js (1)
9-9:⚠️ Potential issue建议在加载模块前验证文件存在性
直接加载编译后的模块可能会因文件不存在而导致运行时错误。建议先验证文件是否存在。
建议修改代码如下：
+const fs = require('fs'); +const path = require('path'); + +const distPath = path.join(__dirname, './dist/index.js'); +if (!fs.existsSync(distPath)) { + throw new Error('构建文件不存在，请确保构建已完成'); +} + require("./dist/index.js");packages/mako/src/binding.d.ts (1)
54-58: 异步生命周期钩子的实现需要验证新增的 writeBundle 和 watchChanges 钩子扩展了插件的能力，但需要确保在异步模块处理时的正确性。
crates/mako/src/generate/transform.rs (2)
Line range hint
38-54: 优化了 transform_all 方法的接口设计移除了
async_deps_map参数简化了接口，提高了代码的可维护性。现在异步依赖直接从 context 中获取，这是一个更好的设计选择。
75-84: 验证线程池的使用是否合理需要确认线程池的配置是否适合当前的工作负载，以及是否需要添加监控指标。
运行以下脚本检查线程池的配置：
✅ Verification successful
Let me gather more information about the thread pool implementation and configuration.
根据搜索结果，我可以生成最终响应了。从代码中可以看到线程池的具体实现和使用情况。
线程池配置合理，无需额外监控
线程池实现使用了 Rayon 库，具有以下特点：
- 采用
OnceLock确保线程池单例模式，避免重复创建- 使用默认配置创建线程池，Rayon 会根据系统 CPU 核心数自动优化
- 提供了
spawn、scope和join等标准接口，使用方式规范当前的线程池使用场景都是短期计算任务（模块转换、依赖解析等），不存在长时间阻塞的情况，因此默认配置已经足够使用。
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # 搜索线程池相关的配置 rg -A 5 'thread_pool::'Length of output: 2425
Script:
#!/bin/bash # Search for thread pool implementation details ast-grep --pattern 'fn spawn($$$) { $$$ }' # Look for any thread pool configuration rg -A 5 'ThreadPool|thread_pool'Length of output: 6357
crates/mako/src/compiler.rs (2)
21-21: 导入声明看起来没有问题！类型导入正确且必要，用于支持新增的异步依赖映射功能。
130-130: 验证异步依赖映射的并发访问安全性新增的
async_deps_map字段在多线程环境下的访问需要特别注意。建议验证以下几点：
- 是否存在死锁风险
- 读写锁的获取顺序是否合理
- 锁的持有时间是否最小化
运行以下脚本来检查相关的并发访问模式：
Also applies to: 366-366
✅ Verification successful
Based on the search results, I can now analyze the thread safety of
async_deps_map:
The
async_deps_mapis accessed in two places:
- Write access in
visitors/async_module.rs: Single write lock when inserting async dependencies- Read access in
generate/transform.rs: Single read lock when reading async dependenciesThe locking pattern shows:
- No nested locks with
async_deps_map- Clean separation between read and write operations
- Short lock durations (locks are released immediately after use)
Let me generate the final response:
异步依赖映射的并发访问是安全的
经过代码分析，
async_deps_map的并发访问实现是合理且安全的：
- 写操作仅在收集异步依赖时进行，使用独立的写锁
- 读操作仅在转换阶段进行，使用独立的读锁
- 锁的作用域被最小化，没有发现嵌套锁的情况
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # 描述：检查 async_deps_map 的并发访问模式 # 预期结果：不应该存在嵌套的锁获取，且锁的作用域应该最小化 # 测试：搜索所有对 async_deps_map 的读写访问 rg -A 3 "async_deps_map\s*\." # 测试：检查是否存在潜在的死锁风险（同时持有多个锁的情况） rg -A 5 "write\(\)|read\(\)" | rg -A 5 "unwrap"Length of output: 44926
crates/mako/src/generate.rs (2)
31-31: 导入语句简化移除了未使用的
Dependency类型导入，这与移除mark_async函数的改动保持一致。
127-128: 验证异步模块转换的正确性
transform_all方法调用已更新以移除异步依赖映射参数。请确保此更改不会影响异步模块的处理逻辑。✅ Verification successful
Let me gather more information about the
transform_allimplementation and its relationship with async module handling.
根据代码分析，我可以生成最终响应了：
transform_all方法的异步处理逻辑已得到验证
transform_all方法移除异步依赖映射参数是安全的，因为：
- 异步模块的处理已经被移至
transform_modules函数中的mark_async步骤- 异步依赖通过
context.async_deps_map全局存储和访问AsyncModule访问器仍然正确处理异步模块的转换，包括：
- 收集异步依赖
- 注入异步处理代码
- 维护模块的执行顺序
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # 描述：验证异步模块的处理逻辑是否仍然正确 # 测试：搜索与异步模块处理相关的代码 echo "检查异步模块处理逻辑：" rg -l "async" --type rust # 测试：检查 transform_all 的实现 echo -e "\n检查 transform_all 实现：" ast-grep --pattern 'fn transform_all'Length of output: 1563
Script:
#!/bin/bash # 检查 transform_all 方法的具体实现 echo "检查 transform_all 方法实现：" rg "transform_all" -A 10 --type rust # 检查异步模块访问器的实现 echo -e "\n检查异步模块访问器：" cat crates/mako/src/visitors/async_module.rs # 检查生成相关的转换逻辑 echo -e "\n检查生成时的转换逻辑：" cat crates/mako/src/generate/transform.rsLength of output: 33214
crates/mako/src/plugins/tree_shaking/shake.rs (3)
24-24: 引入mark_async模块添加了对
mark_async模块的引入，这是为了处理异步模块的标记，符合功能需求。
36-36: 调用mark_async函数在适当的位置调用了
mark_async函数，以标记异步模块，逻辑合理。
49-50:⚠️ Potential issue检查条件表达式的逻辑正确性
条件表达式的逻辑复杂，
&&和||的混合使用可能导致优先级问题，可能出现逻辑错误。建议添加括号明确运算顺序，或者简化条件表达式。例如：
if (module_type != ModuleType::Script || module.is_external() || *is_async) { if (module_type != ModuleType::Script && !module.is_external()) || *is_async { // ... } }请验证逻辑是否符合预期，避免潜在的逻辑错误。
| it("should require looped async moule", () => { | ||
| expect(displayConfig()).toStrictEqual(["key"]) | ||
| }) |
There was a problem hiding this comment.
修复测试描述中的拼写错误并改进异步处理
测试用例存在以下问题：
- 描述中的"moule"应为"module"
- 测试异步模块时应使用 async/await
- 测试描述与实际测试行为不够匹配
建议按如下方式修改：
-it("should require looped async moule", () => {
- expect(displayConfig()).toStrictEqual(["key"])
+it("should handle async module in loop correctly", async () => {
+ expect(await displayConfig()).toStrictEqual(["key"]);📝 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.
| it("should require looped async moule", () => { | |
| expect(displayConfig()).toStrictEqual(["key"]) | |
| }) | |
| it("should handle async module in loop correctly", async () => { | |
| expect(await displayConfig()).toStrictEqual(["key"]); | |
| }) |
| export function listKeys() { | ||
| if(config){ | ||
|
|
||
| return Object.keys(config) | ||
| } | ||
| return ["oops"] | ||
| } |
There was a problem hiding this comment.
建议改进代码实现
-
代码格式问题：
if后缺少空格- 第5行空行可以移除
return语句缩进不一致
-
运行时安全性：
- 建议增加对
config的类型检查 - 当
config为 falsy 值时的处理可能不够严谨
- 建议增加对
建议按如下方式修改：
export function listKeys() {
- if(config){
-
- return Object.keys(config)
+ if (config && typeof config === 'object') {
+ return Object.keys(config)
}
return ["oops"]
}📝 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.
| export function listKeys() { | |
| if(config){ | |
| return Object.keys(config) | |
| } | |
| return ["oops"] | |
| } | |
| export function listKeys() { | |
| if (config && typeof config === 'object') { | |
| return Object.keys(config) | |
| } | |
| return ["oops"] | |
| } |
| import {config} from "./component" | ||
|
|
There was a problem hiding this comment.
💡 Codebase verification
确实存在循环依赖问题
代码分析确认了循环依赖的存在：
utils.js从component.js导入configcomponent.js从utils.js导入listKeyslistKeys函数依赖于config对象displayConfig函数依赖于listKeys函数
建议重构方案：
- 将
config对象移至单独的配置文件中 - 或者将
listKeys函数直接移至component.js，消除跨文件依赖
🔗 Analysis chain
注意潜在的循环依赖问题
根据代码总结，component.js 也导入并使用了 listKeys 函数，这可能会导致循环依赖。建议重构代码以避免这种情况。
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# 验证组件文件中是否存在循环依赖
rg -l "import.*listKeys.*utils" "e2e/fixtures/javascript.async_module_in_loop/component.js"
Length of output: 149
Script:
#!/bin/bash
# Check the actual content of both files to understand the dependency pattern
echo "=== Content of component.js ==="
cat e2e/fixtures/javascript.async_module_in_loop/component.js
echo -e "\n=== Content of utils.js ==="
cat e2e/fixtures/javascript.async_module_in_loop/utils.js
Length of output: 586
| let named = await new Promise((resolve)=>{ | ||
| resolve("named") | ||
| }) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
建议统一 Promise 的实现方式
与上面相同的问题，可以使用更简洁的方式。
建议修改为：
-let named = await new Promise((resolve)=>{
- resolve("named")
-})
+let named = await Promise.resolve("named")📝 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.
| let named = await new Promise((resolve)=>{ | |
| resolve("named") | |
| }) | |
| let named = await Promise.resolve("named") |
| let x = await new Promise((resolve)=>{ | ||
| resolve("default") | ||
| }) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
建议优化 Promise 的创建方式
当 Promise 只是简单地解析一个值时，可以使用更简洁的方式。
建议修改为：
-let x = await new Promise((resolve)=>{
- resolve("default")
-})
+let x = await Promise.resolve("default")📝 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.
| let x = await new Promise((resolve)=>{ | |
| resolve("default") | |
| }) | |
| let x = await Promise.resolve("default") |
| let m = module_graph.get_module_mut(module_id).unwrap(); | ||
| m.info.as_mut().unwrap().is_async = true; |
There was a problem hiding this comment.
建议避免在生产代码中使用unwrap()
在第243行，module_graph.get_module_mut(module_id).unwrap()如果找不到对应的模块，将会导致程序崩溃，这在生产环境中是不安全的。建议使用匹配处理Option类型，以安全地处理可能的None情况。
您可以将代码修改为：
-let m = module_graph.get_module_mut(module_id).unwrap();
+if let Some(m) = module_graph.get_module_mut(module_id) {
+ m.info.as_mut().unwrap().is_async = true;
+ // 继续处理
+} else {
+ // 处理未找到模块的情况，例如记录日志或跳过
+ continue;
+}Committable suggestion was skipped due to low confidence.
| pub fn mark_async(module_graph: &mut ModuleGraph, module_ids: &[ModuleId], context: &Arc<Context>) { | ||
| let mut to_visit_queue = module_graph | ||
| .modules() | ||
| .iter() | ||
| .filter_map(|m| { | ||
| m.info | ||
| .as_ref() | ||
| .and_then(|i| if i.is_async { Some(m.id.clone()) } else { None }) | ||
| }) | ||
| .collect::<VecDeque<_>>(); | ||
| let mut visited = HashSet::new(); | ||
|
|
||
| // polluted async to dependants | ||
| while let Some(module_id) = to_visit_queue.pop_front() { | ||
| if visited.contains(&module_id) { | ||
| continue; | ||
| } | ||
|
|
||
| module_graph | ||
| .get_dependents(&module_id) | ||
| .iter() | ||
| .filter_map(|(dependant, dependency)| { | ||
| if !dependency.resolve_type.is_sync_esm() { | ||
| return None; | ||
| } | ||
| if !visited.contains(*dependant) { | ||
| Some((*dependant).clone()) | ||
| } else { | ||
| None | ||
| } | ||
| }) | ||
| .collect::<Vec<_>>() | ||
| .iter() | ||
| .for_each(|module_id| { | ||
| let m = module_graph.get_module_mut(module_id).unwrap(); | ||
| m.info.as_mut().unwrap().is_async = true; | ||
|
|
||
| to_visit_queue.push_back(module_id.clone()); | ||
| }); | ||
|
|
||
| visited.insert(module_id.clone()); | ||
| } | ||
|
|
There was a problem hiding this comment.
💡 Codebase verification
发现未更新的mark_async调用
在代码库中发现一处调用尚未更新：
crates/mako/src/plugins/tree_shaking/shake.rs中的调用仍使用旧的签名格式：mark_async(module_graph, &module_ids, context)，需要更新为mark_async(module_graph.deref_mut(), &module_ids, context)
🔗 Analysis chain
确认mark_async函数的所有调用已更新
mark_async函数的签名已更改，增加了module_graph的可变引用参数。请确保代码库中所有对mark_async的调用都已更新，传入了新的参数。
运行以下脚本以查找未更新的mark_async调用：
该脚本将帮助您定位可能遗漏更新的函数调用，以确保新签名在整个代码库中一致使用。
Let me analyze the actual usage and definition of the mark_async function to verify if all calls have been properly updated.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# 描述：查找未传入`module_graph`参数的`mark_async`函数调用
# 期望结果：所有调用都包含`module_graph`参数
rg 'mark_async\s*\(\s*module_ids' -A1
Length of output: 39
Script:
#!/bin/bash
# 搜索所有mark_async的调用
rg "mark_async\(" -A 2
# 搜索mark_async函数的定义
ast-grep --pattern 'fn mark_async($$$)'
Length of output: 919
| module_ids.iter().for_each(|module_id| { | ||
| let deps = module_graph.get_dependencies_info(module_id); | ||
| let mut async_deps_map = context.async_deps_map.write().unwrap(); |
There was a problem hiding this comment.
处理写锁获取失败的情况
在第254行，使用了context.async_deps_map.write().unwrap()，如果获取写锁失败（例如发生PoisonError），程序将会崩溃。为了提高代码的健壮性，建议处理可能的错误，提供适当的错误处理逻辑。
您可以将代码修改为：
-let mut async_deps_map = context.async_deps_map.write().unwrap();
+let mut async_deps_map = match context.async_deps_map.write() {
+ Ok(lock) => lock,
+ Err(e) => {
+ // 处理获取锁失败的情况，例如记录错误并返回
+ eprintln!("Failed to acquire write lock: {}", e);
+ return;
+ }
+};Committable suggestion was skipped due to low confidence.
| use crate::{mako_profile_function, mako_profile_scope}; | ||
|
|
||
| type TreeShakingModuleMap = HashMap<ModuleId, RefCell<TreeShakeModule>>; | ||
|
|
||
| pub fn optimize_modules(module_graph: &mut ModuleGraph, context: &Arc<Context>) -> Result<()> { | ||
| let module_ids = { | ||
| let (mut module_ids, _) = module_graph.toposort(); | ||
| // start from the leaf nodes, so reverser the sort |
There was a problem hiding this comment.
修正注释中的拼写错误
注释中“reverser”应为“reverse”，请修正拼写错误。
建议修改为：
- // start from the leaf nodes, so reverser the sort
+ // start from the leaf nodes, so reverse the sort📝 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.
| // start from the leaf nodes, so reverser the sort | |
| // start from the leaf nodes, so reverse the sort |
| let (_skipped, tree_shake_modules_ids): (Vec<ModuleId>, Vec<ModuleId>) = | ||
| topo_sorted_modules.into_iter().partition(|module_id| { | ||
| let module = module_graph.get_module(module_id).unwrap(); | ||
|
|
||
| let module_type = module.get_module_type(); | ||
|
|
||
| // skip non script modules and external modules | ||
| if module_type != ModuleType::Script || module.is_external() { | ||
| if module_type != ModuleType::Script && !module.is_external() { | ||
| let is_async = &module.info.as_ref().unwrap().is_async; |
There was a problem hiding this comment.
避免使用可能导致崩溃的 unwrap()
使用 unwrap() 可能会在 module.info 为 None 时导致程序崩溃。建议处理 Option 类型以提高代码的健壮性。
可以考虑修改为：
-let is_async = &module.info.as_ref().unwrap().is_async;
+let is_async = module.info.as_ref().map_or(&false, |info| &info.is_async);📝 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.
| let is_async = &module.info.as_ref().unwrap().is_async; | |
| let is_async = module.info.as_ref().map_or(&false, |info| &info.is_async); |
Summary by CodeRabbit
新功能
JsHooks接口，增加了新的方法和属性以支持模块构建和监控变更。文档
mako.config.json配置文件，指定了主入口文件。