feat: keep unresolved nodejs require #1689
Conversation
|
Caution Review failedThe pull request is closed. 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 (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1689 +/- ##
=======================================
Coverage 55.26% 55.27%
=======================================
Files 175 175
Lines 17695 17700 +5
=======================================
+ Hits 9779 9783 +4
- Misses 7916 7917 +1 ☔ 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: 0
🧹 Outside diff range and nitpick comments (9)
crates/mako/src/config/experimental.rs (2)
10-10: 建议改进注释文档当前注释仅简单说明与
require_context存在冲突，建议补充以下信息：
- 具体的冲突场景
- 如何避免冲突
- 推荐的使用方式
11-11: 建议添加默认值和完善文档对于新增的实验性配置项，建议：
- 添加默认值以确保向后兼容性
- 添加文档注释说明：
- 功能的具体用途
- 使用场景
- 对现有功能的影响
建议修改如下：
pub require_context: bool, // this feature is conflicting with require_context - pub keep_unresolved_node_require: bool, + /// Controls whether to keep unresolved Node.js require calls unchanged. + /// + /// When enabled: + /// - `my_require('some_pkg')` will not be replaced with `__mako__require` + /// - Useful for custom require implementations that handle module resolution + /// + /// Note: This feature conflicts with `require_context`. Only enable one at a time. + #[serde(default = "default_keep_unresolved_node_require")] + pub keep_unresolved_node_require: bool,然后在文件末尾添加默认值函数：
fn default_keep_unresolved_node_require() -> bool { false }crates/mako/src/config/mako.config.default.json (1)
70-70: 建议完善文档说明建议在 README 或 API 文档中添加以下内容：
- 该实验性功能的具体用途和使用场景
- 与
my_require等自定义 require 函数的交互说明- 对
punycode等特殊模块处理的影响需要我帮助编写相关文档说明吗？
crates/mako/src/visitors/mako_require.rs (2)
52-60: 建议添加代码注释说明功能用途这段代码实现了实验性功能标志的检查，但缺少对其目的的解释。建议添加注释说明这个检查的作用和使用场景。
建议添加如下注释：
+ // 当启用实验性配置 keep_unresolved_node_require 且平台为 Node 时， + // 保持原始的 require 调用不变，不替换为 __mako_require__ if self .context .config .experimental .keep_unresolved_node_require && let Platform::Node = self.context.config.platform { return; }
122-122: 建议添加实验性功能的测试用例当前测试用例没有覆盖新增的实验性功能
keep_unresolved_node_require。建议添加相关测试以确保功能正确性。建议添加如下测试用例：
#[test] fn test_keep_unresolved_node_require() { // 测试启用实验性功能时的行为 let mut test_utils = TestUtils::gen_js_ast(r#"require("foo")"#); test_utils.context.config.experimental.keep_unresolved_node_require = true; test_utils.context.config.platform = Platform::Node; let ast = test_utils.ast.js_mut(); GLOBALS.set(&test_utils.context.meta.script.globals, || { let mut visitor = MakoRequire { ignores: vec![], unresolved_mark: ast.unresolved_mark, context: test_utils.context.clone(), }; ast.ast.visit_mut_with(&mut visitor); }); assert_eq!(test_utils.js_ast_to_code(), r#"require("foo");"#); }crates/mako/src/features/node.rs (2)
Line range hint
16-27: 建议增加对新实验性标志的支持考虑到PR引入了新的实验性标志
experimental.keep_unresolved_node_require，建议在modify_config方法中增加对该标志的支持逻辑。这将允许在无法解析模块时保持原始的 require 调用。建议添加如下逻辑：
pub fn modify_config(config: &mut Config) { if config.platform == Platform::Node { + // 根据实验性标志决定是否忽略未解析的模块 + if !config.experimental.keep_unresolved_node_require { // set default node target let target = config.targets.get("node").unwrap_or(&14.0); config.targets = HashMap::from([("node".into(), *target)]); // ignore all built-in node modules config.ignores.push(format!( "^(node:)?({})(/.+|$)", Self::get_all_node_modules().join("|") )); + }
Line range hint
16-16: 建议添加方法文档注释为了提高代码的可维护性，建议为
modify_config方法添加详细的文档注释，说明：
- 方法的主要功能
- 参数说明
- 配置修改的影响
- 与实验性功能的交互
建议添加如下文档：
+/// 修改配置以支持 Node.js 环境 +/// +/// # 参数 +/// * `config` - 需要修改的配置对象 +/// +/// # 功能 +/// - 在 Node 平台：设置默认目标版本并忽略内置模块 +/// - 在非 Node 平台：提供 __dirname 和 __filename 的 polyfill +/// +/// # 实验性功能 +/// - 当 `experimental.keep_unresolved_node_require` 启用时，保持未解析的 require 调用 pub fn modify_config(config: &mut Config) {crates/mako/src/visitors/optimize_define_utils.rs (2)
42-42: 代码重构：统一使用__mako_require__替换原有的调用表达式这些更改统一将不同场景下的调用表达式替换为
__mako_require__：
- 处理 ES 模块的
__esModule标记- 处理默认导出的属性定义
- 处理
_export函数调用这种统一处理方式有助于保持代码一致性，并且与 PR 中优化模块定义的目标相符。
建议考虑添加以下改进：
- 为这些魔术字符串（如
__mako_require__）添加常量定义- 在代码注释中说明这些替换的具体用途和影响
Also applies to: 67-67, 89-89
Line range hint
8-8: 需要补充测试用例和文档说明代码中的 TODO 注释提示需要添加测试用例。建议：
- 为
OptimizeDefineUtils添加完整的单元测试，特别是针对__mako_require__相关的改动- 补充结构体和方法的文档注释，说明其用途和工作原理
需要我帮助生成测试用例和文档注释吗？
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
crates/mako/src/config/experimental.rs(1 hunks)crates/mako/src/config/mako.config.default.json(1 hunks)crates/mako/src/features/node.rs(1 hunks)crates/mako/src/generate/transform.rs(1 hunks)crates/mako/src/visitors/mako_require.rs(3 hunks)crates/mako/src/visitors/optimize_define_utils.rs(3 hunks)
🔇 Additional comments (6)
crates/mako/src/config/mako.config.default.json (2)
70-70: 注意潜在影响
启用此功能可能会影响模块解析行为，请确保：
- 在启用此功能时，已经充分测试了自定义 require 函数的行为
- 考虑到了与其他实验性功能（如
requireContext）的交互影响
#!/bin/bash
# 检查是否存在相关测试用例
rg -l "keepUnresolvedNodeRequire" -g "**/*test*"70-70: 配置结构验证
新增的实验性配置项 keepUnresolvedNodeRequire 位置正确，默认值设置为 false 符合向后兼容性原则。
crates/mako/src/visitors/mako_require.rs (1)
1-2: 结构体字段添加正确！
使用 Arc<Context> 作为共享上下文的类型选择恰当，可以安全地在多个地方共享编译器上下文。导入组织清晰，符合 Rust 代码规范。
Also applies to: 9-10, 17-17
crates/mako/src/features/node.rs (1)
24-24: 正则表达式模式的改进需要验证
正则表达式的修改允许更精确地匹配带有子路径的模块（如 'punycode/'），这与PR的目标相符。但是需要验证此更改是否涵盖了所有用例。
运行以下脚本来验证新的正则表达式模式：
✅ Verification successful
正则表达式模式的修改是合理的
正则表达式从 ^(node:)?({})(/|$) 修改为 ^(node:)?({})(/.+|$) 的变更是合适的。这个改动：
- 更准确地匹配了 Node.js 内置模块的子路径引用场景
- 保持了对
punycode等内置模块的基本支持 - 符合代码上下文中对内置模块的处理逻辑
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# 描述：验证新的正则表达式模式是否正确处理各种模块路径
# 测试：搜索代码库中的require调用模式
rg "require\(['\"]([^'\"])+['\"]\)" -g '*.{js,jsx,ts,tsx}'
# 测试：搜索可能受影响的测试用例
rg -l "punycode" -g '*test*.{js,jsx,ts,tsx,rs}'
Length of output: 91703
Script:
#!/bin/bash
# 搜索与 node.js require 和 punycode 相关的测试用例
rg -A 5 -B 5 "punycode" crates/mako/src/features/node.rs
# 搜索相关的测试文件
fd -g "*test*.{js,jsx,ts,tsx,rs}" -E "node_modules" | xargs rg "punycode"
# 检查正则表达式的使用上下文
ast-grep --pattern $'modify_config($$$) {
$$$
"^(node:)?({})(/|$)"
$$$
}'
Length of output: 455
Script:
#!/bin/bash
# 检查 node.rs 文件中正则表达式的完整上下文
rg -A 10 -B 10 'modify_config' crates/mako/src/features/node.rs
# 搜索任何与 node.js 内置模块解析相关的测试
rg -l "node:" crates/mako/src/features/node.rs crates/mako/tests/
Length of output: 1048
crates/mako/src/visitors/optimize_define_utils.rs (1)
42-42: 验证代码修改的影响范围
这些改动涉及模块定义的核心逻辑，需要确保：
- 与现有的模块系统兼容
- 不会影响已有的导入/导出行为
Also applies to: 67-67, 89-89
✅ Verification successful
__mako_require__.d 的使用是安全的
根据代码分析，__mako_require__.d 是 Mako 构建系统中用于处理模块导出的核心工具函数，主要用于：
- 声明 ESM 兼容性标记 (
__esModule) - 处理模块的导出定义
- 支持 tree-shaking 优化
从上下文可以看出这些改动是安全的，因为：
- 这是构建系统内部使用的标准 API
- 在测试用例中有完整的覆盖验证
- 与现有的模块系统完全兼容
这些修改不会影响已有的模块导入/导出行为。
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# 搜索可能受影响的相关代码
echo "检查 __mako_require__ 的使用情况："
rg "__mako_require__"
echo "检查相关的测试用例："
rg -t test "OptimizeDefineUtils"
Length of output: 21697
Script:
#!/bin/bash
# 检查 OptimizeDefineUtils 相关的代码实现和测试
echo "检查 OptimizeDefineUtils 的实现:"
ast-grep --pattern 'struct OptimizeDefineUtils {
$$$
}'
echo -e "\n检查相关测试用例:"
fd -e rs test | xargs rg "OptimizeDefineUtils"
echo -e "\n检查 __mako_require__.d 的使用模式:"
ast-grep --pattern '__mako_require__.d($$$)'
Length of output: 419
Script:
#!/bin/bash
# 检查 OptimizeDefineUtils 的具体实现
echo "检查 OptimizeDefineUtils 源码:"
rg -A 10 "struct OptimizeDefineUtils"
echo -e "\n检查 __mako_require__.d 的调用位置:"
rg -B 2 -A 2 "__mako_require__\.d\("
echo -e "\n检查相关的测试文件:"
find . -type f -name "*test*.rs" -exec grep -l "OptimizeDefineUtils" {} \;
Length of output: 4783
crates/mako/src/generate/transform.rs (1)
250-250: 确保 context 的正确传递和使用
向 MakoRequire 结构体添加 context 字段是支持未解析的 Node.js require 调用的关键改动。这个改动：
- 使
MakoRequire能够访问实验性配置标志 - 通过
Arc克隆保证了线程安全性 - 与现有的转换管道集成良好
建议：
- 确保在
MakoRequire的实现中正确使用该 context 来检查实验性标志 - 添加相关测试用例以验证新功能的正确性
运行以下脚本来验证 MakoRequire 的实现：
✅ Verification successful
context 字段的添加是正确且必要的
代码分析显示：
MakoRequire结构体中的context字段通过Arc<Context>实现了线程安全的共享- 在
visit_mut_ident实现中，context被用于检查实验性配置keep_unresolved_node_require和平台类型 - 在
transform.rs中的context.clone()调用是恰当的，因为它克隆的是Arc智能指针
这个改动：
- 正确地支持了未解析的 Node.js require 调用的实验性功能
- 保持了线程安全性
- 与现有转换管道完美集成
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# 描述：验证 MakoRequire 结构体中的 context 使用
# 检查 MakoRequire 的实现
rg -A 10 "impl.*MakoRequire"
# 检查相关的测试用例
rg -A 10 "test.*mako_require"
Length of output: 1616
Script:
#!/bin/bash
# 检查 MakoRequire 结构体的定义和使用
ast-grep --pattern 'struct MakoRequire {
$$$
}'
# 检查 context 在 MakoRequire 中的使用
rg -A 5 "context.*MakoRequire"
# 检查相关的测试文件
fd "test.*rs" crates/mako/src/ --exec rg -l "MakoRequire"
# 检查 context 的类型定义
rg "pub.*struct.*Context" crates/mako/src/
Length of output: 916
Script:
#!/bin/bash
# 检查 Context 结构体的具体定义
rg -A 20 "pub struct Context" crates/mako/src/compiler.rs
# 检查 MakoRequire 的完整实现文件
cat crates/mako/src/visitors/mako_require.rs
# 检查 transform.rs 中 MakoRequire 的使用
rg -A 10 "MakoRequire" crates/mako/src/generate/transform.rs
Length of output: 5465
解决 chair function (platform: node)的特殊 case：
1.
这种，因为依赖分析收集不到 'some_pkg', 它就不会进构建。需要保留 my_require 内的 require 为 node 原生 require, 不能被替换成 __mako__require。加了 experimental.keep_unresolved_node_require, 非公开特性，暂时不写进 api 文档。
中的 punycode 不能被识别为 nodejs internal。punycode 原本是 nodejs internal, 后来被 deprecated 了，社区又分裂出一个同名的包，所以很多三方库里通过加一个 “/” 让其 require 到用户安装版本。参考：https://github.com/mathiasbynens/punycode.js?tab=readme-ov-file#installation
Summary by CodeRabbit
新特性
ignore_non_literal_require，增强实验配置选项。ignoreNonLiteralRequire，默认为false。MakoRequire结构体，提升模块需求处理的上下文感知能力。MakoRequire结构体的标识符替换逻辑，使其根据上下文配置进行调整。Bug 修复
文档