-
Notifications
You must be signed in to change notification settings - Fork 77
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
feat: support webpackIgnore and makoIgnore magic comment #1636
Conversation
Walkthrough本次更改涉及对多个文件的修改,以增强对动态导入和忽略插件的支持。 Changes
Assessment against linked issues
Possibly related PRs
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 (
|
There was a problem hiding this 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 (5)
e2e/fixtures/magic-comments.ignore/expect.js (2)
6-7
: 魔法注释的断言测试实现得很好。代码正确检查了 'webpackIgnore' 和 'makoIgnore' 魔法注释的存在。这些测试确保了在构建过程中正确处理了魔法注释。
建议:为了保持一致性,可以考虑将两个断言的错误消息格式统一。例如:
assert(index.includes(`import(/* webpackIgnore: true */ "./foo");`), "应包含 webpackIgnore 注释"); assert(index.includes(`import(/* makoIgnore: true */ "./bar");`), "应包含 makoIgnore 注释");这样可以使错误消息更加明确和统一。
1-7
: 总体来说,这是一个优秀的测试文件。该文件成功实现了对魔法注释支持的测试,涵盖了 'webpackIgnore' 和 'makoIgnore' 两种情况。代码结构清晰,易于理解,并且与 PR 的目标很好地对齐。这为确保构建过程中正确处理这些注释提供了坚实的基础。
建议:
- 考虑添加更多的边界情况测试,例如检查没有魔法注释的导入是否被正确处理。
- 可以考虑添加一些注释来解释测试的目的和预期结果,这将有助于其他开发者理解和维护这些测试。
crates/mako/src/module.rs (1)
132-132
: 新增ignore
字段以支持忽略导入选项在
ImportOptions
结构体中添加了ignore: bool
字段。这个改动与 PR 的目标一致,即支持webpackIgnore
和makoIgnore
魔法注释。建议:
- 考虑为
ignore
字段添加文档注释,说明其用途和预期行为。- 更新
ImportOptions
的Default
实现,为ignore
字段设置适当的默认值。crates/mako/src/compiler.rs (1)
331-336
: 代码变更看起来不错,但可以考虑进行小优化这个变更简化了
IgnorePlugin
的处理逻辑,使其更加一致和易于维护。错误处理也很恰当。不过,我们可以考虑在
config.ignores
为空时避免不必要的IgnorePlugin
创建:if !config.ignores.is_empty() { let ignores = config .ignores .iter() .map(|ignore| Regex::new(ignore).map_err(Error::new)) .collect::<Result<Vec<Regex>>>()?; plugins.push(Arc::new(plugins::ignore::IgnorePlugin { ignores })); }这样可以在没有忽略规则时略微提高性能。
crates/mako/src/visitors/dep_analyzer.rs (1)
Line range hint
243-291
: 建议为ignore
选项添加单元测试目前的测试用例中未包含对
ignore
选项的验证。为了确保新功能的可靠性和正确性,建议在测试模块中添加针对ignore
选项的测试用例。如果您需要帮助,我可以协助编写这些测试用例,或者为此创建一个新的 GitHub issue。
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (8)
- crates/mako/src/compiler.rs (1 hunks)
- crates/mako/src/module.rs (1 hunks)
- crates/mako/src/plugins/ignore.rs (2 hunks)
- crates/mako/src/visitors/dep_analyzer.rs (2 hunks)
- crates/mako/src/visitors/dynamic_import.rs (1 hunks)
- e2e/fixtures/magic-comments.ignore/expect.js (1 hunks)
- e2e/fixtures/magic-comments.ignore/mako.config.json (1 hunks)
- e2e/fixtures/magic-comments.ignore/src/index.ts (1 hunks)
✅ Files skipped from review due to trivial changes (2)
- e2e/fixtures/magic-comments.ignore/mako.config.json
- e2e/fixtures/magic-comments.ignore/src/index.ts
🧰 Additional context used
🔇 Additional comments (5)
e2e/fixtures/magic-comments.ignore/expect.js (2)
1-3
: 导入语句和设置看起来不错!代码正确导入了所需的模块,并使用了自定义工具函数来解析构建结果。这种方法显示了良好的代码组织和可重用性。
5-5
: 文件内容检索实现得当。代码从解析后的构建结果中正确获取了 'index.js' 的内容。变量名 'index' 清晰明了,有助于理解代码意图。
crates/mako/src/plugins/ignore.rs (1)
7-7
: 导入ResolveType
模块以支持新的依赖类型判断正确地引入了
ResolveType
,以便在后续代码中根据依赖的resolve_type
进行匹配和处理。crates/mako/src/visitors/dep_analyzer.rs (2)
61-69
: 实现了对魔法注释ignore
选项的解析新增的代码正确解析了
makoIgnore
和webpackIgnore
魔法注释中的ignore
选项,并将结果存储在ImportOptions
的ignore
字段中,逻辑清晰,符合预期。
240-243
: 正则表达式函数正确匹配魔法注释
get_magic_comment_ignore_regex
函数正确地创建了用于匹配makoIgnore
和webpackIgnore
魔法注释的正则表达式,能够有效提取true
或false
的值。
deps.retain(|dep| { | ||
if self.ignores.iter().any(|ig| ig.is_match(&dep.source)) { | ||
return false; | ||
} | ||
if let ResolveType::DynamicImport(import_options) = &dep.resolve_type { | ||
if import_options.ignore { | ||
return false; | ||
} | ||
} | ||
true | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
建议简化条件判断逻辑以提升代码可读性
可以合并嵌套的条件判断,使代码更加简洁明了。例如:
deps.retain(|dep| {
if self.ignores.iter().any(|ig| ig.is_match(&dep.source)) {
return false;
}
- if let ResolveType::DynamicImport(import_options) = &dep.resolve_type {
- if import_options.ignore {
- return false;
- }
- }
+ if matches!(&dep.resolve_type, ResolveType::DynamicImport(import_options) if import_options.ignore) {
+ return false;
+ }
true
});
这样可以减少嵌套,提高代码的可读性和维护性。
📝 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.
deps.retain(|dep| { | |
if self.ignores.iter().any(|ig| ig.is_match(&dep.source)) { | |
return false; | |
} | |
if let ResolveType::DynamicImport(import_options) = &dep.resolve_type { | |
if import_options.ignore { | |
return false; | |
} | |
} | |
true | |
}); | |
deps.retain(|dep| { | |
if self.ignores.iter().any(|ig| ig.is_match(&dep.source)) { | |
return false; | |
} | |
if matches!(&dep.resolve_type, ResolveType::DynamicImport(import_options) if import_options.ignore) { | |
return false; | |
} | |
true | |
}); |
let resolved_info = self.dep_to_replace.resolved.get(source.value.as_ref()); | ||
|
||
// e.g. | ||
// import(/* webpackIgnore: true */ "foo") | ||
// will be ignored | ||
if resolved_info.is_none() { | ||
return; | ||
} | ||
|
||
let resolved_info = resolved_info |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
建议使用 if let
简化代码并避免变量遮蔽
当前代码中,先获取 resolved_info
,然后检查是否为 None
,再使用 unwrap()
解包。这种方式会导致重复声明 resolved_info
,造成变量遮蔽,影响代码可读性。可以使用 if let
语句来简化逻辑,同时避免显式调用 unwrap()
。
建议修改如下:
- let resolved_info = self.dep_to_replace.resolved.get(source.value.as_ref());
- // e.g.
- // import(/* webpackIgnore: true */ "foo")
- // will be ignored
- if resolved_info.is_none() {
- return;
- }
- let resolved_info = resolved_info
- // If the identifier is not in dep_to_replace.missing,
- // it must be resolved, so unwrap is safe here.
- .unwrap();
+ if let Some(resolved_info) = self.dep_to_replace.resolved.get(source.value.as_ref()) {
+ // 如果标识符不在 dep_to_replace.missing 中,那么一定能够解析,因此这里是安全的
+ // 处理 resolved_info 的逻辑
+ } else {
+ // e.g.
+ // import(/* webpackIgnore: true */ "foo")
+ // will be ignored
+ return;
+ }
📝 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 resolved_info = self.dep_to_replace.resolved.get(source.value.as_ref()); | |
// e.g. | |
// import(/* webpackIgnore: true */ "foo") | |
// will be ignored | |
if resolved_info.is_none() { | |
return; | |
} | |
let resolved_info = resolved_info | |
if let Some(resolved_info) = self.dep_to_replace.resolved.get(source.value.as_ref()) { | |
// 如果标识符不在 dep_to_replace.missing 中,那么一定能够解析,因此这里是安全的 | |
// 处理 resolved_info 的逻辑 | |
} else { | |
// e.g. | |
// import(/* webpackIgnore: true */ "foo") | |
// will be ignored | |
return; | |
} |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1636 +/- ##
=======================================
Coverage 55.53% 55.54%
=======================================
Files 172 172
Lines 17384 17401 +17
=======================================
+ Hits 9655 9666 +11
- Misses 7729 7735 +6 ☔ View full report in Codecov by Sentry. |
Close #1622
Summary by CodeRabbit
新功能
ignore
字段到ImportOptions
结构,允许用户指示是否忽略特定的导入选项。mako.config.json
,包含代码拆分和实验性功能的设置。src/index.ts
中添加了动态导入语句,以支持条件加载模块。测试
expect.js
,用于验证使用魔法注释的导入语句。改进
IgnorePlugin
和DepAnalyzer
的逻辑,增强了对动态导入的处理。