Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: hash not stable (&revert import * as optimize) #1610

Merged
merged 10 commits into from
Oct 12, 2024

Conversation

Jinbao1001
Copy link
Member

@Jinbao1001 Jinbao1001 commented Sep 24, 2024

Summary by CodeRabbit

  • 新功能

    • 引入了新的模块 collect_explicit_prop,扩展了树摇插件的功能。
    • 新增了 IdExplicitPropAccessCollector 结构体,用于分析 JavaScript 代码中的显式属性访问。
    • 添加了优化命名空间导入的功能。
    • 新增了三个导出函数 shouldKeep1shouldKeep2shouldNotKeep
    • 更新了树摇插件中的 transform_js 方法,以支持新的树摇配置。
  • 测试

    • 新增了测试文件 expect.js,验证树摇功能的正确性。
    • 新增了配置文件 mako.config.json,设置模块连接选项。

Copy link
Contributor

coderabbitai bot commented Sep 24, 2024

Walkthrough

此次更改涉及多个文件的更新,主要集中在 CI 工作流的环境变量设置、模块图中依赖管理的优化、新增树摇插件功能、以及显式属性访问收集器的实现。此外,还引入了新的测试文件和配置文件,以支持模块优化和验证功能。整体上,这些更改旨在提升代码的可维护性和测试能力。

Changes

文件路径 更改摘要
.github/workflows/ci.yml 在 CI 工作流的 "Test E2E" 步骤中添加了环境变量 RUST_BACKTRACE,设置为 full
crates/mako/src/module_graph.rs 修改了 ModuleGraph 中的依赖管理逻辑,移除了 remove_dependency_module_by_source_and_resolve_type 方法,新增了 rewrite_dependency 方法以简化依赖更新过程。
crates/mako/src/plugins/tree_shaking.rs 新增模块 collect_explicit_prop,扩展了树摇插件的功能。
crates/mako/src/plugins/tree_shaking/collect_explicit_prop.rs 引入 IdExplicitPropAccessCollector 结构体,分析 JavaScript 代码中的显式属性访问,并实现了相关方法和测试。
crates/mako/src/plugins/tree_shaking/remove_useless_stmts.rs 新增 optimize_import_namespace 函数,优化命名空间导入,并更新 remove_useless_stmts 函数以调用此新函数。
crates/mako/src/plugins/tree_shaking/shake.rs 修改 optimize_modules 函数,新增 HELPERS 变量的上下文设置。
crates/mako/src/plugins/tree_shaking/shake/skip_module.rs 更新 skip_module_optimize 函数,新增 context 参数并简化依赖处理逻辑。
e2e/fixtures/tree-shaking.import_namespace/expect.js 新增测试文件 expect.js,验证模块导入的预期行为。
e2e/fixtures/tree-shaking.import_namespace/mako.config.json 新增配置文件 mako.config.json,设置模块连接优化选项为 false
e2e/fixtures/tree-shaking.import_namespace/src/index.tsx 新增 index.tsx 文件,演示如何导入和使用模块的导出函数。
e2e/fixtures/tree-shaking.import_namespace/src/mod.js 新增 mod.js 文件,导出函数 shouldKeep1shouldKeep2shouldNotKeep

Possibly related PRs

Suggested reviewers

  • xusd320
  • stormslowly

Poem

🐰 在代码的花园中跳跃,
新功能如花儿绽放,
树摇插件添新彩,
测试更稳妥无妨。
兔子欢呼声响亮,
代码世界更辉煌! 🌼


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?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 4

Outside diff range and nitpick comments (7)
e2e/fixtures/tree-shaking.import_namespace/src/mod.js (1)

5-7: 函数实现正确,但可以优化表达。

shouldKeep2 函数实现了输入值的加倍操作。函数名同样暗示在树摇过程中应该保留此函数。

建议将实现改为更直观的乘法操作:

export function shouldKeep2(x) {
-  return x + x;
+  return 2 * x;
}

这样可以更清晰地表达函数的意图,即将输入值乘以2。

e2e/fixtures/tree-shaking.import_namespace/expect.js (1)

12-14: 测试断言看起来正确,但可以增加一些注释

测试断言检查了"shouldKeep1"和"shouldKeep2"的存在,以及"shouldNotKeep"的不存在,这看起来是在验证树摇(tree-shaking)的有效性。这些断言清晰明了。

建议添加一些注释来解释这些字符串的含义,以及为什么它们应该被保留或移除。这将有助于其他开发者更好地理解测试的目的。例如:

// 检查应该被保留的代码
expect(content).toContain("shouldKeep1");
expect(content).toContain("shouldKeep2");

// 检查应该被树摇掉的代码
expect(content).not.toContain("shouldNotKeep");
e2e/fixtures/tree-shaking.import_namespace/src/index.tsx (3)

8-14: 数组操作示例良好,但可以进行小优化

这段代码很好地展示了ES6+的数组操作特性,包括初始化、解构和展开语法。"Guardian don't remove"的注释表明这些操作是有意保留的。

对于小型内联数组,可以考虑移除尾随逗号,使代码更加简洁:

-let array= [1,2,3,]
+let array = [1, 2, 3]

另外,建议在变量声明和等号两侧保持一致的空格,以提高可读性。


16-21: 对象操作示例良好,建议保持一致性

这段代码很好地展示了ES6+的对象操作特性,包括初始化、解构和展开语法。操作正确且与之前的数组操作保持一致。

为了与数组初始化保持一致,建议在对象初始化时也使用尾随逗号:

-let object = {a: 1,b: 2,c: 3}
+let object = { a: 1, b: 2, c: 3, }

同时,建议在对象属性的冒号后添加空格,以提高可读性。这样的一致性有助于维护代码风格的统一。


1-21: 文件结构良好,但缺乏明确目的说明

这个文件很好地展示了现代JavaScript/TypeScript的特性,包括模块导入、解构和展开语法等。代码结构清晰,易于理解。

然而,文件的具体用途在更大的应用程序上下文中并不明确。建议:

  1. 添加文件顶部的注释,解释此文件的目的(例如,是否为测试文件、示例代码等)。
  2. 如果这是一个示例或测试文件,考虑将其移动到专门的示例或测试目录中。
  3. 如果这是生产代码的一部分,请重新评估console.log语句的使用,并考虑添加更多的业务逻辑或将其与其他组件集成。

这些改进将有助于其他开发人员更好地理解和维护这段代码。

crates/mako/src/module_graph.rs (1)

317-329: 代码重构提高了可读性和效率

这个重构很好地改进了代码。使用retain方法使代码更加简洁和惯用。引入found标志确保只移除一个匹配的依赖项,这是一个很好的做法。

考虑添加一个注释,解释为什么在处理一个边之后就立即返回。例如:

// 假设每个模块只有一个匹配的依赖项,所以在处理一个边后立即返回
return;

这将有助于未来的开发者理解这个设计决策。

crates/mako/src/plugins/tree_shaking/collect_explicit_prop.rs (1)

31-32: 改进注释的可读性以提高代码理解度。

第31-32行的注释内容不够清晰,建议重新表述以更好地传达含义,便于他人理解。

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 68de8e2 and 198dff8.

Files selected for processing (10)
  • .github/workflows/ci.yml (1 hunks)
  • crates/mako/src/module_graph.rs (1 hunks)
  • crates/mako/src/plugins/tree_shaking.rs (1 hunks)
  • crates/mako/src/plugins/tree_shaking/collect_explicit_prop.rs (1 hunks)
  • crates/mako/src/plugins/tree_shaking/remove_useless_stmts.rs (3 hunks)
  • crates/mako/src/plugins/tree_shaking/shake.rs (2 hunks)
  • e2e/fixtures/tree-shaking.import_namespace/expect.js (1 hunks)
  • e2e/fixtures/tree-shaking.import_namespace/mako.config.json (1 hunks)
  • e2e/fixtures/tree-shaking.import_namespace/src/index.tsx (1 hunks)
  • e2e/fixtures/tree-shaking.import_namespace/src/mod.js (1 hunks)
Additional comments not posted (10)
e2e/fixtures/tree-shaking.import_namespace/src/mod.js (2)

1-3: 函数实现正确,符合预期用途。

shouldKeep1 函数实现了一个简单的平方运算。函数名暗示在树摇过程中应该保留此函数,这对于测试或演示树摇功能很有用。


9-11: 函数实现正确,但需要澄清其用途。

shouldNotKeep 函数实现了一个简单的立方运算。函数名暗示在树摇过程中应该移除此函数。

考虑到函数名表明它应该在树摇过程中被移除,请澄清包含此函数的目的。是否是为了验证树摇机制能够正确地识别和移除未使用的代码?

为了验证这个假设,我们可以运行以下脚本来检查项目中是否有使用这个函数的地方:

如果这个函数确实应该被移除,那么在实际的应用代码中不应该找到它的使用。

Verification successful

验证通过,函数 shouldNotKeep 仅在测试中使用,不影响生产环境。

  • shouldNotKeep 函数仅在 e2e/fixtures/tree-shaking.import_namespace/expect.js 中被引用。
Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# 描述:搜索项目中是否有使用 shouldNotKeep 函数的地方

# 测试:搜索 shouldNotKeep 函数的使用。预期:没有匹配项或仅在测试文件中找到。
rg --type js 'shouldNotKeep'

Length of output: 218

e2e/fixtures/tree-shaking.import_namespace/expect.js (1)

1-10: 导入语句和设置看起来不错!

导入必要的模块和实用函数,解析构建结果并注入Jest设置的过程看起来都很合理。这为后续的测试提供了良好的基础。

e2e/fixtures/tree-shaking.import_namespace/src/index.tsx (1)

5-6: 注意控制台日志在生产环境中的使用

这些控制台日志语句正确地调用了导入的函数,但在生产环境中可能会引起问题。

请确保在将代码部署到生产环境之前,移除或注释掉这些控制台日志语句。如果这些日志是为了调试目的,考虑使用更高级的日志系统或条件性日志记录。

crates/mako/src/plugins/tree_shaking.rs (1)

12-12: 新模块添加:collect_explicit_prop

新增了 collect_explicit_prop 模块,这可能扩展了树摇(tree shaking)插件的功能。

建议进一步调查以下几点:

  1. 这个新模块的具体实现和用途是什么?
  2. 它如何与现有的树摇功能集成?
  3. 是否需要在 FarmTreeShake 结构体中添加相应的调用?

请运行以下脚本以验证新模块的使用情况:

Verification successful

建议进一步检查 collect_explicit_prop 模块的实现,以确保其功能与树摇插件的整体逻辑一致。请运行以下脚本以获取该模块的代码内容:


模块 collect_explicit_prop 添加验证通过

collect_explicit_prop 模块已正确集成并在相关文件中使用,功能实现符合预期,且相关测试覆盖充分。

Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# 描述:检查 collect_explicit_prop 模块的使用情况

# 测试:搜索 collect_explicit_prop 的使用。预期:找到该模块的实现和使用位置。
rg --type rust -e "collect_explicit_prop" -e "use (self::)?collect_explicit_prop::" crates/mako/src/

Length of output: 293


Script:

#!/bin/bash
# 描述:获取 collect_explicit_prop 模块的实现

cat crates/mako/src/plugins/tree_shaking/collect_explicit_prop.rs

Length of output: 6338

.github/workflows/ci.yml (1)

118-119: 添加 RUST_BACKTRACE 环境变量以增强调试能力

在 E2E 测试步骤中添加 RUST_BACKTRACE=full 环境变量是一个很好的做法。这将有助于在测试失败时提供更详细的错误信息。

考虑事项:

  1. 这可能会增加日志输出的大小,特别是在发生错误时。
  2. 在某些情况下,完整的回溯可能会暴露敏感信息,尽管在 CI 环境中这通常不是问题。

建议:

  • 确保团队成员了解这一变化,以便他们能够更好地解读 CI 日志。
  • 考虑是否需要在所有环境中都启用完整回溯,或者只在特定的调试构建中启用。
crates/mako/src/plugins/tree_shaking/collect_explicit_prop.rs (1)

93-95: 请确认代码兼容的最低Rust版本。

第93-95行使用的if let ... && let ...语法是在Rust 1.65.0版本中引入的。请确保项目的最低Rust版本要求满足此语法的使用。

crates/mako/src/plugins/tree_shaking/remove_useless_stmts.rs (1)

118-118: 请验证新函数调用的影响

在第118行,添加了对 optimize_import_namespace 函数的调用。请确保此更改不会对现有功能产生负面影响,并在相关情况下进行了充分的测试。

crates/mako/src/plugins/tree_shaking/shake.rs (2)

14-14: 引入必要的模块。

新增了对HelpersHELPERS的引入,为后续使用HELPERS.set提供了支持,合理且必要。


146-153: HELPERS上下文中调用shake_module函数。

shake_module函数的调用包裹在HELPERS.set(&Helpers::new(true), || { ... })中,确保在执行期间拥有一个新的Helpers实例,有助于正确管理代码生成的辅助函数,避免状态共享导致的潜在问题。

Comment on lines +1 to +5
{
"optimization": {
"concatenateModules": false
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

💡 Codebase verification

配置文件中的 concatenateModules 设置不一致

在多个 mako.config.json 文件中发现 concatenateModules 设置为 truefalse,这可能导致项目中不同部分的优化行为不一致。例如:

  • examples/with-antd/mako.config.json 设置为 true
  • e2e/fixtures/tree-shaking.import_namespace/mako.config.json 设置为 false
  • 其他多个配置文件中也存在类似不一致的设置

建议统一 concatenateModules 的配置,以确保整个项目的优化策略一致,提升构建过程的可预测性和稳定性。

Analysis chain

配置文件添加正确,建议验证优化策略。

新增的 mako.config.json 文件结构正确,设置了 concatenateModulesfalse,这将禁用模块连接优化。

请验证此配置是否符合项目的优化目标。可以运行以下脚本来检查其他相关配置文件中是否有类似的设置:

这将帮助确保新配置与项目的整体优化策略保持一致。

Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# 描述:搜索其他配置文件中的 concatenateModules 设置

# 测试:搜索其他包含 concatenateModules 设置的 JSON 文件
echo "搜索其他包含 concatenateModules 设置的配置文件:"
rg --type json 'concatenateModules' -g '!e2e/fixtures/tree-shaking.import_namespace/mako.config.json'

# 测试:检查是否有其他与树摇相关的配置
echo "搜索其他可能与树摇相关的配置:"
rg --type json 'tree.?shaking|optimization' -g '*.config.json'

Length of output: 23073

Comment on lines +1 to +3
import * as mod from "./mod";

const { shouldKeep2 } = mod
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

考虑优化导入和解构语句

当前的导入和解构方式是正确的,但可以进行优化以提高代码的简洁性和可读性。

建议将导入和解构合并为一个语句,如下所示:

-import * as mod from "./mod";
-
-const { shouldKeep2 } = mod
+import { shouldKeep1, shouldKeep2 } from "./mod";

这样可以减少代码行数,并且更明确地表示我们只需要 mod 模块中的特定函数。

Committable suggestion was skipped due to low confidence.

Comment on lines +272 to +273
let mut shadow = module.clone();

Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

优化性能:避免克隆整个模块

在第272行,代码克隆了整个模块 let mut shadow = module.clone();。克隆大型模块可能带来性能开销。建议考虑是否有办法在不克隆整个模块的情况下实现相同的功能,或者仅克隆必要的部分,以提升性能。

Comment on lines +265 to +266
let (sym, ctxt) = ns.rsplit_once('#').unwrap();
(sym.into(), SyntaxContext::from_u32(ctxt.parse().unwrap()))
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

避免使用 unwrap() 导致潜在的崩溃

在第265行和266行,代码直接使用了 unwrap() 方法。如果 ns 字符串中不包含 #,或者 ctxt.parse() 解析失败,程序将会崩溃。建议使用模式匹配来处理可能的错误情况,提升代码的健壮性。

您可以修改代码如下:

-            let (sym, ctxt) = ns.rsplit_once('#').unwrap();
-            (sym.into(), SyntaxContext::from_u32(ctxt.parse().unwrap()))
+            if let Some((sym, ctxt_str)) = ns.rsplit_once('#') {
+                if let Ok(ctxt_num) = ctxt_str.parse() {
+                    (sym.into(), SyntaxContext::from_u32(ctxt_num))
+                } else {
+                    // 处理解析失败的情况
+                }
+            } else {
+                // 处理未找到 '#' 的情况
+            }

Committable suggestion was skipped due to low confidence.

Copy link

codecov bot commented Sep 24, 2024

Codecov Report

Attention: Patch coverage is 72.83237% with 47 lines in your changes missing coverage. Please review.

Project coverage is 55.66%. Comparing base (428cdfa) to head (7447872).
Report is 1 commits behind head on master.

Files with missing lines Patch % Lines
...o/src/plugins/tree_shaking/remove_useless_stmts.rs 37.20% 27 Missing ⚠️
...mako/src/plugins/tree_shaking/shake/skip_module.rs 0.00% 11 Missing ⚠️
crates/mako/src/module_graph.rs 0.00% 5 Missing ⚠️
.../src/plugins/tree_shaking/collect_explicit_prop.rs 96.19% 4 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1610      +/-   ##
==========================================
+ Coverage   55.44%   55.66%   +0.22%     
==========================================
  Files         171      172       +1     
  Lines       17142    17294     +152     
==========================================
+ Hits         9504     9627     +123     
- Misses       7638     7667      +29     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 2

Outside diff range and nitpick comments (1)
crates/mako/src/module_graph.rs (1)

307-314: 新方法实现更加高效,但可以考虑添加错误处理和注释

这个新的 rewrite_dependency 方法的实现看起来更加高效和直接。它通过先删除所有现有的依赖关系,然后添加新的依赖关系,有效地重写了模块的依赖关系。

然而,有几点建议可以进一步改进这个方法:

  1. 考虑添加错误处理机制。如果在执行过程中出现错误(例如,在删除旧依赖后添加新依赖时失败),可能会导致模块处于不一致的状态。

  2. 建议添加方法注释,解释该方法的目的和行为,这将有助于其他开发者理解和维护代码。

建议按照以下方式修改代码:

/// 重写指定模块的所有依赖关系。
/// 
/// 该方法首先移除模块的所有现有依赖,然后添加新的依赖关系。
/// 
/// # 参数
/// 
/// * `module_id` - 要重写依赖的模块ID
/// * `deps` - 新的依赖关系列表
/// 
/// # 错误
/// 
/// 如果在重写过程中出现错误,将回滚所有更改并返回错误。
pub fn rewrite_dependency(&mut self, module_id: &ModuleId, deps: Vec<(&ModuleId, Dependency)>) -> Result<(), Error> {
    let old_deps = self.get_dependencies(module_id);
    
    // 移除所有现有依赖
    let mut edges = self.get_edges(module_id, Direction::Outgoing);
    while let Some((edge_index, _node_index)) = edges.next(&self.graph) {
        self.graph.remove_edge(edge_index);
    }
    
    // 尝试添加新依赖
    for (m, d) in deps {
        if let Err(e) = self.add_dependency(module_id, m, d.clone()) {
            // 如果出错,回滚更改
            self.rewrite_dependency(module_id, old_deps)?;
            return Err(e);
        }
    }
    
    Ok(())
}

这个修改版本添加了错误处理和回滚机制,以及详细的方法注释。

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 198dff8 and 19bfccd.

Files selected for processing (2)
  • crates/mako/src/module_graph.rs (2 hunks)
  • crates/mako/src/plugins/tree_shaking/shake/skip_module.rs (4 hunks)

Comment on lines 332 to 333
let deps = analyze_deps::AnalyzeDeps::analyze_deps(&info.ast, &info.file, context.clone());
deps.unwrap().resolved_deps.iter().for_each(|r| {
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

避免使用unwrap()造成的潜在崩溃

在第332行,直接调用deps.unwrap()可能在depsNone时引发panic,导致程序崩溃。建议处理Result类型的错误,使用?操作符或模式匹配来安全地处理可能的错误。

应用以下修改以处理可能的错误:

-    let deps = analyze_deps::AnalyzeDeps::analyze_deps(&info.ast, &info.file, context.clone());
-    deps.unwrap().resolved_deps.iter().for_each(|r| {
+    let deps = analyze_deps::AnalyzeDeps::analyze_deps(&info.ast, &info.file, context.clone())?;
+    deps.resolved_deps.iter().for_each(|r| {

并确保在函数签名中传播错误或进行适当的错误处理。

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 deps = analyze_deps::AnalyzeDeps::analyze_deps(&info.ast, &info.file, context.clone());
deps.unwrap().resolved_deps.iter().for_each(|r| {
let deps = analyze_deps::AnalyzeDeps::analyze_deps(&info.ast, &info.file, context.clone())?;
deps.resolved_deps.iter().for_each(|r| {

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between 19bfccd and b4335d1.

Files selected for processing (2)
  • crates/mako/src/module_graph.rs (2 hunks)
  • crates/mako/src/plugins/tree_shaking/shake/skip_module.rs (4 hunks)
Files skipped from review as they are similar to previous changes (1)
  • crates/mako/src/plugins/tree_shaking/shake/skip_module.rs
Additional comments not posted (1)
crates/mako/src/module_graph.rs (1)

312-314: 添加新依赖的实现方式清晰

您使用 iter().for_each 方法添加新的依赖,代码简洁明了,符合 Rust 的惯用风格。

Comment on lines +307 to +310
pub fn rewrite_dependency(&mut self, module_id: &ModuleId, deps: Vec<(ModuleId, Dependency)>) {
let mut edges = self.get_edges(module_id, Direction::Outgoing);

while let Some((edge_index, _node_index)) = edges.next(&self.graph) {
let dependencies = self.graph.edge_weight_mut(edge_index).unwrap();

if let Some(to_del_dep) = dependencies
.iter()
.position(|dep| *source == dep.source && dep.resolve_type.same_enum(&resolve_type))
{
dependencies.take(&dependencies.iter().nth(to_del_dep).unwrap().clone());

if dependencies.is_empty() {
self.graph.remove_edge(edge_index);
}
return;
}
self.graph.remove_edge(edge_index);
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

在迭代过程中修改图结构可能导致未定义行为

rewrite_dependency 方法中,您在迭代 edges 的同时删除边,这可能导致迭代器失效和未定义行为。建议在迭代之前先收集所有需要删除的边的索引,然后再进行删除操作。

建议修改代码如下:

let edges_to_remove: Vec<_> = self.get_edges(module_id, Direction::Outgoing)
    .filter_map(|(edge_index, _)| Some(edge_index))
    .collect();

for edge_index in edges_to_remove {
    self.graph.remove_edge(edge_index);
}

@Jinbao1001 Jinbao1001 changed the title [WIP] fix: revert(revert import * as optimize) & hash not stable fix: hash not stable (&revert import * as optimize) Sep 25, 2024
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Outside diff range and nitpick comments (1)
crates/mako/src/module_graph.rs (1)

307-314: 重写依赖关系的方法实现得当,但可以进一步优化

新添加的 rewrite_dependency 方法很好地解决了之前在迭代过程中修改图结构可能导致的未定义行为问题。这种方法通过先删除所有现有的出边,然后重新添加新的依赖关系,避免了在迭代过程中修改图结构。

然而,我们可以通过以下方式进一步优化性能:

  1. 使用 self.graph.edges_directed() 替代 self.get_edges(),这样可以直接获取边的引用,避免额外的迭代器封装。
  2. 使用 retain 方法来移除边,这样可以在一次遍历中完成所有边的移除操作。

建议按照以下方式重构代码:

pub fn rewrite_dependency(&mut self, module_id: &ModuleId, deps: Vec<(ModuleId, Dependency)>) {
    let node_index = *self.id_index_map.get(module_id).expect("Module not found");
    self.graph.retain_edges(|graph, edge_index| {
        graph.edge_endpoints(edge_index).map_or(true, |(source, _)| source != node_index)
    });
    
    for (m, d) in deps {
        self.add_dependency(module_id, &m, d);
    }
}

这个优化版本减少了不必要的迭代,并使用了更高效的图操作方法。

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between c0b9e5e and 1993386.

Files selected for processing (2)
  • crates/mako/src/module_graph.rs (2 hunks)
  • crates/mako/src/plugins/tree_shaking/shake/skip_module.rs (5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • crates/mako/src/plugins/tree_shaking/shake/skip_module.rs

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 2

🧹 Outside diff range and nitpick comments (2)
crates/mako/src/plugins/tree_shaking.rs (2)

Line range hint 51-90: 优化模块项替换的逻辑

visit_mut_module_items 方法中,当前实现先遍历收集需要替换的项,然后反向替换模块项列表。可以考虑直接在遍历过程中进行替换,避免反向操作,简化代码并提高效率。


Line range hint 117-129: 测试用例输入与期望输出相同

test_non_toplevel_decl 测试中,输入代码和期望输出代码相同,没有验证代码的任何变更。建议检查测试用例是否正确,或修改测试以验证非顶层声明不被拆分的行为。

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between 1993386 and 7447872.

📒 Files selected for processing (2)
  • crates/mako/src/plugins/tree_shaking.rs (1 hunks)
  • crates/mako/src/plugins/tree_shaking/shake/skip_module.rs (5 hunks)
🧰 Additional context used
🔇 Additional comments (6)
crates/mako/src/plugins/tree_shaking.rs (1)

Line range hint 26-36: 确认对 config._tree_shaking 的访问是否正确

transform_js 方法中,代码检查了 context.config._tree_shaking 是否有值。请确认 Config 结构体中是否存在名为 _tree_shaking 的字段,或者是否应使用 tree_shaking

可以运行以下脚本来验证 Config 结构体中的字段命名:

crates/mako/src/plugins/tree_shaking/shake/skip_module.rs (5)

15-15: 导入新模块 analyze_deps

代码中引入了 analyze_deps 模块,这对于依赖分析的功能实现是必要的。


170-170: 优化参数命名提升可读性

将参数名从 _context 修改为 context,符合命名惯例,增强了代码的可读性和一致性。


186-186: 优化参数命名提升可读性

apply_replace 函数中的参数从 _context 修改为 context,与其他部分保持一致,提高了代码的清晰度。


331-340: 避免使用 unwrap() 可能引发的潜在崩溃

第 334 行直接调用了 deps.unwrap(),如果 depsNone,将导致程序 panic 崩溃。建议安全地处理可能的错误,使用 ? 操作符或匹配模式来处理 Result 类型。


497-497: 保持参数命名一致

在调用 apply_replace 函数时,将参数名更新为 context,与函数定义保持一致,增强了代码的一致性。

@@ -9,6 +9,7 @@ use crate::compiler::Context;
use crate::module_graph::ModuleGraph;
use crate::plugin::{Plugin, PluginTransformJsParam};

mod collect_explicit_prop;
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

建议移除未使用的模块 collect_explicit_prop

collect_explicit_prop 模块在代码中未被引用,建议移除以保持代码整洁。

) {
let stmt_id = to_replace.0;
let replaces = &to_replace.1;
let source = &to_replace.2;
let _source = &to_replace.2;
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

移除未使用的变量 _source

变量 _source 被声明但未在函数中使用,建议删除以简化代码。

请应用以下修改:

-            let _source = &to_replace.2;
📝 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 _source = &to_replace.2;

@Jinbao1001 Jinbao1001 merged commit 1c79bd4 into master Oct 12, 2024
21 checks passed
@Jinbao1001 Jinbao1001 deleted the fix/hash-not-stable branch October 12, 2024 08:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant