Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions crates/mako/src/generate/transform.rs
Original file line number Diff line number Diff line change
Expand Up @@ -78,8 +78,9 @@ pub fn transform_modules_in_thread(
let module_id = module_id.clone();
let async_deps = async_deps_by_module_id
.get(&module_id)
.expect(&module_id.id)
.clone();
.cloned()
.unwrap_or(vec![]);

thread_pool::spawn(move || {
let module_graph = context.module_graph.read().unwrap();
let deps = module_graph.get_dependencies(&module_id);
Expand Down
82 changes: 60 additions & 22 deletions crates/mako/src/visitors/async_module.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
use std::collections::HashMap;
use std::collections::{HashMap, HashSet, VecDeque};
use std::sync::Arc;

use swc_core::common::util::take::Take;
Expand Down Expand Up @@ -211,23 +211,61 @@ pub fn mark_async(
) -> HashMap<ModuleId, Vec<Dependency>> {
let mut async_deps_by_module_id = HashMap::new();
let mut module_graph = context.module_graph.write().unwrap();
// TODO: 考虑成环的场景

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;
Comment on lines +248 to +249
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

注意:unwrap()可能引发潜在的崩溃

在第248行,let m = module_graph.get_module_mut(module_id).unwrap();中使用了unwrap()。如果get_module_mut返回None,程序将会崩溃。建议添加错误处理以防止这种情况:

建议修改如下:

-let m = module_graph.get_module_mut(module_id).unwrap();
+if let Some(m) = module_graph.get_module_mut(module_id) {
+    // 后续操作
+} else {
+    // 处理模块不存在的情况,例如记录日志或跳过
+}

Committable suggestion was skipped due to low confidence.


Comment on lines +249 to +250
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

注意:再次使用unwrap()可能引发潜在的崩溃

第249行的m.info.as_mut().unwrap().is_async = true;中,再次使用了unwrap()。如果infoNone,程序将崩溃。建议增加对info的存在性检查:

建议修改如下:

-if let Some(m) = module_graph.get_module_mut(module_id) {
-    m.info.as_mut().unwrap().is_async = true;
+if let Some(m) = module_graph.get_module_mut(module_id) {
+    if let Some(info) = m.info.as_mut() {
+        info.is_async = true;
+    } else {
+        // 处理info为None的情况,例如记录日志或设置默认值
+    }
+} else {
+    // 处理模块不存在的情况
+}

Committable suggestion was skipped due to low confidence.

to_visit_queue.push_back(module_id.clone());
});

visited.insert(module_id.clone());
}

module_ids.iter().for_each(|module_id| {
let deps = module_graph.get_dependencies_info(module_id);
let async_deps: Vec<Dependency> = deps
.into_iter()
.filter(|(_, dep, is_async)| dep.resolve_type.is_sync_esm() && *is_async)
.map(|(_, dep, _)| dep.clone())
.collect();
let module = module_graph.get_module_mut(module_id).unwrap();
if let Some(info) = module.info.as_mut() {
// a module with async deps need to be polluted into async module
if !info.is_async && !async_deps.is_empty() {
info.is_async = true;
}
if !async_deps.is_empty() {
async_deps_by_module_id.insert(module_id.clone(), async_deps);
}
});

async_deps_by_module_id
}

Expand Down Expand Up @@ -255,8 +293,8 @@ add(1, 2);
"#
.trim());
assert_eq!(
code,
r#"
code,
r#"
__mako_require__._async(module, async (handleAsyncDeps, asyncResult)=>{
"use strict";
Object.defineProperty(exports, "__esModule", {
Expand All @@ -273,7 +311,7 @@ __mako_require__._async(module, async (handleAsyncDeps, asyncResult)=>{
asyncResult();
}, true);
"#.trim()
);
);
}

#[test]
Expand All @@ -286,8 +324,8 @@ console.log(foo)
"#
.trim());
assert_eq!(
code,
r#"
code,
r#"
__mako_require__._async(module, async (handleAsyncDeps, asyncResult)=>{
"use strict";
Object.defineProperty(exports, "__esModule", {
Expand All @@ -308,8 +346,8 @@ __mako_require__._async(module, async (handleAsyncDeps, asyncResult)=>{
asyncResult();
}, true);
"#
.trim()
);
.trim()
);
}

#[test]
Expand All @@ -321,8 +359,8 @@ add(1, 2);
"#
.trim());
assert_eq!(
code,
r#"
code,
r#"
__mako_require__._async(module, async (handleAsyncDeps, asyncResult)=>{
"use strict";
Object.defineProperty(exports, "__esModule", {
Expand All @@ -340,8 +378,8 @@ __mako_require__._async(module, async (handleAsyncDeps, asyncResult)=>{
asyncResult();
}, true);
"#
.trim()
);
.trim()
);
}

#[test]
Expand Down Expand Up @@ -374,8 +412,8 @@ const async = require('./miexed_async');
"#
.trim());
assert_eq!(
code,
r#"
code,
r#"
__mako_require__._async(module, async (handleAsyncDeps, asyncResult)=>{
"use strict";
Object.defineProperty(exports, "__esModule", {
Expand All @@ -393,7 +431,7 @@ __mako_require__._async(module, async (handleAsyncDeps, asyncResult)=>{
asyncResult();
}, true);
"#.trim()
);
);
}

fn run(js_code: &str) -> String {
Expand Down
11 changes: 11 additions & 0 deletions e2e/fixtures/javascript.async_module_in_loop/async.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
let x = await new Promise((resolve)=>{
resolve("default")
})
Comment on lines +1 to +3
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

建议优化 Promise 的创建方式

当前的 Promise 创建方式过于冗长。对于简单的值,可以使用更简洁的方式。

建议使用以下方式之一:

-let x = await new Promise((resolve)=>{
-    resolve("default")
-})
+let x = await Promise.resolve("default")

或者更简单的:

-let x = await new Promise((resolve)=>{
-    resolve("default")
-})
+let x = "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.

Suggested change
let x = await new Promise((resolve)=>{
resolve("default")
})
let x = await Promise.resolve("default")


let named = await new Promise((resolve)=>{
resolve("named")
})

export default x;

export {named}
11 changes: 11 additions & 0 deletions e2e/fixtures/javascript.async_module_in_loop/component.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
import { listKeys } from "./utils"

import { named } from "./async"

export const config = {
key: "value"
}

export function displayConfig() {
return listKeys()
}
9 changes: 9 additions & 0 deletions e2e/fixtures/javascript.async_module_in_loop/expect.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
const {
injectSimpleJest,
parseBuildResult,
moduleDefinitionOf,
} = require("../../../scripts/test-utils");
const { files } = parseBuildResult(__dirname);
injectSimpleJest();
Comment on lines +6 to +7
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

建议增加错误处理机制

在解析构建结果时,建议添加错误处理来提高测试的健壮性。

建议修改为:

-const { files } = parseBuildResult(__dirname);
+let files;
+try {
+  ({ files } = parseBuildResult(__dirname));
+} catch (error) {
+  console.error('构建结果解析失败:', error);
+  throw error;
+}
📝 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.

Suggested change
const { files } = parseBuildResult(__dirname);
injectSimpleJest();
let files;
try {
({ files } = parseBuildResult(__dirname));
} catch (error) {
console.error('构建结果解析失败:', error);
throw error;
}
injectSimpleJest();


require("./dist/index.js");
6 changes: 6 additions & 0 deletions e2e/fixtures/javascript.async_module_in_loop/index.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
import {displayConfig} from "./component";

it("should require looped async moule", () => {
expect(displayConfig()).toStrictEqual(["key"])
})
Comment on lines +3 to +5
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

测试用例需要进行一些改进

以下几点建议:

  1. 测试描述中 "moule" 应该改为 "module"
  2. 由于测试的是异步模块,应该使用 async/await 语法
  3. 建议添加错误处理

建议按如下方式修改代码:

-it("should require looped async moule", () => {
-    expect(displayConfig()).toStrictEqual(["key"])
-})
+it("should require looped async module", async () => {
+    try {
+        const result = await displayConfig();
+        expect(result).toStrictEqual(["key"]);
+    } catch (error) {
+        throw new Error(`测试失败: ${error.message}`);
+    }
+});
📝 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.

Suggested change
it("should require looped async moule", () => {
expect(displayConfig()).toStrictEqual(["key"])
})
it("should require looped async module", async () => {
try {
const result = await displayConfig();
expect(result).toStrictEqual(["key"]);
} catch (error) {
throw new Error(`测试失败: ${error.message}`);
}
});


5 changes: 5 additions & 0 deletions e2e/fixtures/javascript.async_module_in_loop/mako.config.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
{
"entry": {
"index": "./index.js"
}
}
9 changes: 9 additions & 0 deletions e2e/fixtures/javascript.async_module_in_loop/utils.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
import {config} from "./component"
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codebase verification

存在循环依赖问题需要注意

经验证,确实存在循环依赖关系:

  • utils.js./component 导入 config
  • component.js./utils 导入 listKeys

这种循环依赖可能会在异步模块加载时造成问题:

  • 模块初始化顺序不确定
  • 可能出现未定义的引用
  • 影响代码的可维护性

建议:

  • 考虑重构代码结构,打破循环依赖
  • 可以将共享的配置抽取到单独的配置文件中
  • 或者使用依赖注入的方式重组代码
🔗 Analysis chain

验证循环依赖关系

由于该文件从 ./component 导入 config,需要确认是否存在循环依赖,以确保异步模块的正确处理。

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# 描述:检查是否存在循环依赖
# 预期:查找 component.js 中是否引用了 utils.js

rg -l "import.*from.*['\"]\.\/utils['\"]" "$(dirname $(fd -t f utils.js))/component.js"

Length of output: 341


Script:

#!/bin/bash
# 检查 component.js 中是否引用了 utils.js
rg "import.*from.*[\"']./utils[\"']" "$(dirname $(fd -t f utils.js))/component.js"

# 查看 component.js 的完整内容以分析依赖关系
cat "$(dirname $(fd -t f utils.js))/component.js"

Length of output: 612


Script:

#!/bin/bash
# 修复路径问题,正确查找和检查文件
fd -t f "component.js" "e2e/fixtures/javascript.async_module_in_loop" -x cat {}

Length of output: 250


export function listKeys() {
if(config){

return Object.keys(config)
}
Comment on lines +4 to +7
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

改进代码格式和错误处理

代码存在以下问题:

  1. if 条件后的括号间距不一致
  2. 第5行空行可以删除
  3. 缺少对 config 类型的检查

建议按如下方式修改:

-    if(config){
-        
-    return Object.keys(config)
+    if (config && typeof config === 'object') {
+        return Object.keys(config)
📝 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.

Suggested change
if(config){
return Object.keys(config)
}
if (config && typeof config === 'object') {
return Object.keys(config)
}

return ["oops"]
}