-
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: add watch_changes plugin hook #1651
Conversation
Walkthrough此拉取请求引入了多个结构和方法的更新,主要集中在 Changes
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: 1
🧹 Outside diff range and nitpick comments (12)
crates/binding/src/js_plugin.rs (1)
125-135
: 新的watch_changes
方法实现得很好!该方法正确地实现了
Plugin
trait 的要求,并且恰当地处理了钩子的调用。以下是一些观察和建议:
- 方法签名与
Plugin
trait 的要求一致。- 在调用钩子之前,代码正确地检查了钩子是否存在。
- 方法适当地处理了钩子调用的结果。
建议:
- 考虑为未使用的
_context
参数添加一个简短的注释,说明它可能在将来使用。这样可以避免其他开发者误认为这是一个错误或冗余参数。您是否希望我为
_context
参数生成一个注释示例?crates/binding/src/js_hook.rs (3)
62-65
: 新增的 watch_changes 钩子函数看起来不错!这个新增的可选字段为插件系统提供了监听文件变化的能力,增强了系统的灵活性。函数签名设计得当,支持同步和异步操作。
建议在代码注释中添加一个简短的说明,解释这个钩子函数的用途和触发时机,以提高代码的可读性和可维护性。例如:
/// 当文件系统发生变化时触发的钩子函数。 /// 可用于实现自定义的文件监听逻辑。 #[napi( ts_type = "(id: string, { event: 'create' | 'delete' | 'update' }) => Promise<void> | void;" )] pub watch_changes: Option<JsFunction>,
91-91
: TsFnHooks 结构体中的 watch_changes 字段实现得很好这个新增的字段与
JsHooks
结构体中的watch_changes
相对应,使用ThreadsafeFunction
确保了跨线程调用的安全性。为了提高代码的可读性和一致性,建议将这个新字段移到结构体定义的顶部,紧跟在其他核心钩子函数(如
build_start
、build_end
等)之后。这样可以更好地组织相关的钩子函数。例如:pub struct TsFnHooks { pub build_start: Option<ThreadsafeFunction<(), ()>>, pub build_end: Option<ThreadsafeFunction<(), ()>>, pub write_bundle: Option<ThreadsafeFunction<(), ()>>, pub generate_end: Option<ThreadsafeFunction<Value, ()>>, pub watch_changes: Option<ThreadsafeFunction<(String, WatchChangesParams), ()>>, // ... 其他字段 }
153-157
: WatchChangesParams 结构体定义合理这个新的结构体为
watch_changes
钩子函数提供了清晰的参数类型定义,有助于提高代码的类型安全性和可读性。为了进一步提高类型安全性和代码的自文档化能力,建议使用枚举类型来表示事件类型,而不是使用字符串。这样可以在编译时捕获潜在的错误,并提供更好的 IDE 支持。例如:
#[napi(object, use_nullable = true)] pub struct WatchChangesParams { pub event: WatchChangeEvent, } #[napi] pub enum WatchChangeEvent { Create, Delete, Update, }这样的修改将确保只有有效的事件类型可以被传递,并且在 TypeScript 中也会有更好的类型提示。
crates/mako/src/plugin.rs (2)
154-156
: 新增的watch_changes
方法看起来不错,但建议添加文档注释。这个新方法的签名和默认实现看起来很合理。它允许插件选择性地实现这个功能,而不会破坏现有的插件。
建议为这个方法添加文档注释,解释其用途、参数含义以及预期的使用场景。例如:
/// 监听文件变化的方法。 /// /// # 参数 /// * `id` - 变化的资源标识符 /// * `event` - 变化事件的类型(例如:"add", "remove", "modify") /// * `context` - 编译上下文 /// /// # 返回值 /// 返回 `Result<()>`,表示处理是否成功 fn watch_changes(&self, _id: &str, _event: &str, _context: &Arc<Context>) -> Result<()> { Ok(()) }
342-347
:PluginDriver
中的watch_changes
实现看起来不错,但可以考虑优化。这个实现正确地遍历了所有插件并调用它们的
watch_changes
方法。错误处理也很恰当,在遇到第一个错误时就会停止并返回。考虑使用
Iterator::try_for_each
来简化代码:pub fn watch_changes(&self, id: &str, event: &str, context: &Arc<Context>) -> Result<()> { self.plugins.iter().try_for_each(|plugin| plugin.watch_changes(id, event, context)) }这样可以使代码更简洁,同时保持相同的功能。
docs/config.zh-CN.md (3)
Line range hint
575-585
: 新增的watch
配置选项很有用这个新增的配置选项允许用户指定在文件监视过程中要忽略的路径,这对于提高性能很有帮助。示例清晰明了。
建议在示例中添加一个多路径忽略的情况,以展示更复杂的用法。例如:
{ watch: { ignorePaths: ["foo", "bar/node_modules"], }, }
Line range hint
587-591
: 新增的writeToDisk
配置选项提供了灵活性这个新增的布尔类型配置选项允许用户控制是否在开发模式下将构建结果写入磁盘,默认值为
true
。这为开发者提供了更多的灵活性。建议稍微扩展一下描述,解释一下为什么用户可能想要禁用这个选项。例如:
是否在开发模式下将构建结果写入磁盘。设置为 `false` 可以提高性能,特别是在处理大型项目时,但会使得直接检查构建输出变得困难。
575-575
: 新增的watchChanges
插件属性增强了文件监视功能这个新增的
watchChanges
属性允许插件响应文件变化,提供了更细粒度的控制。这与新增的watch
配置选项相呼应,增强了系统的整体文件监视能力。建议在文档中添加一个简单的示例,以展示如何使用这个新属性。例如:
plugins: [ { name: 'my-plugin', watchChanges: (id, { event }) => { console.log(`File ${id} was ${event}ed`); }, }, ]这将帮助开发者更好地理解如何实现和使用这个新功能。
crates/mako/src/dev.rs (1)
Line range hint
366-424
: 建议进一步优化错误处理
rebuild
方法的整体改进很好,特别是在错误处理方面。然而,我注意到方法中其他部分的错误处理可能还有改进的空间。建议考虑对整个方法采用一致的错误处理方式,可能的话使用Result
类型来传播错误。这样可以进一步提高代码的可读性和可维护性。如果您需要帮助实现这些建议,我可以提供更具体的代码示例。您是否希望我为整个
rebuild
方法提供一个优化后的版本?docs/config.md (2)
578-579
: 完善watchChanges
钩子函数的参数说明建议为
watchChanges
钩子函数的参数提供更详细的说明,以帮助用户更好地理解和使用这个新功能。建议添加以下说明:
watchChanges?: (id: string, params: { event: "create" | "delete" | "update" }) => void; +// id: 发生变化的文件路径 +// params.event: 变化类型,可能是 "create"(创建)、"delete"(删除)或 "update"(更新)
Line range hint
580-594
: 新增watch
配置项的文档说明新增的
watch
配置项文档清晰地说明了其用途和配置方法。这个配置项允许用户指定在监视过程中需要忽略的路径。为了使文档更加完善,建议添加以下内容:
- 解释为什么用户可能需要忽略某些路径。
- 提供更多示例,包括如何忽略多个路径或使用通配符。
建议添加以下内容:
watch: { ignorePaths: ["foo"], }, }
+忽略路径可以帮助提高监视性能,特别是对于大型项目或包含大量生成文件的目录。
+
+更多示例:
+
+```ts
+{
- watch: {
- // 忽略多个路径
- ignorePaths: ["foo", "bar/baz"],
- // 使用通配符忽略所有 .test.js 文件
- ignorePaths: ["**/*.test.js"],
- },
+}
+```
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
- crates/binding/src/js_hook.rs (4 hunks)
- crates/binding/src/js_plugin.rs (2 hunks)
- crates/mako/src/dev.rs (1 hunks)
- crates/mako/src/dev/update.rs (1 hunks)
- crates/mako/src/plugin.rs (2 hunks)
- docs/config.md (1 hunks)
- docs/config.zh-CN.md (1 hunks)
🧰 Additional context used
🔇 Additional comments (9)
crates/binding/src/js_plugin.rs (2)
11-12
: 导入更改看起来不错!
WatchChangesParams
的导入与新添加的watch_changes
方法的使用一致。这个更改是必要的,并且没有引入任何问题。
Line range hint
1-180
: 总体评价:代码变更实现良好,增强了插件系统的功能这次变更主要添加了
watch_changes
功能,使插件系统能够响应变化事件。具体观察如下:
- 新增的
watch_changes
方法与现有代码风格和结构保持一致。- 导入更改和新方法的实现都经过了仔细考虑,没有引入明显的问题。
- 这个新功能增强了插件系统的灵活性,允许插件对文件变化做出反应。
总的来说,这是一个有价值的添加,它扩展了插件系统的能力,同时保持了代码库的整体质量和一致性。
crates/binding/src/js_hook.rs (1)
Line range hint
1-179
: 总体评价:代码质量良好,新功能实现合理本次修改引入了
watch_changes
钩子函数及相关结构,显著增强了插件系统处理文件系统变化的能力。代码实现利用了 Rust 的类型系统和 NAPI 绑定,整体结构清晰,类型安全。主要亮点:
- 新增的
watch_changes
钩子函数设计灵活,支持同步和异步操作。- 使用
ThreadsafeFunction
确保了跨线程调用的安全性。WatchChangesParams
结构体提供了清晰的参数类型定义。建议改进:
- 添加简短的代码注释,解释新增钩子函数的用途和触发时机。
- 考虑重新组织
TsFnHooks
结构体中字段的顺序,以提高可读性。- 使用枚举类型替代字符串来表示事件类型,进一步提高类型安全性。
总的来说,这些修改是一个很好的补充,提高了系统的灵活性和功能性。在实施建议的小改进后,代码质量将更上一层楼。
crates/mako/src/plugin.rs (1)
Line range hint
1-424
: 总结:watch_changes
功能是一个很好的补充,但需要更多上下文。新增的
watch_changes
方法为插件系统增加了监听资源变化的能力,这是一个很有价值的功能。实现遵循了 Rust 的最佳实践,代码质量很好。然而,没有提供关于如何使用这个新功能的上下文信息。建议:
- 在 PR 描述中解释这个功能的用途和使用场景。
- 更新相关文档,说明如何在插件中实现和使用
watch_changes
。- 考虑添加单元测试来验证这个新功能的正确性。
为了确保这个功能的正确集成,请运行以下脚本来检查是否有其他地方调用了这个新方法:
这将帮助我们理解这个新功能是如何被集成到系统中的。
✅ Verification successful
验证通过:
watch_changes
方法已在相关文件中成功调用。
crates/mako/src/dev/update.rs
中调用了watch_changes
方法 3 次🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # 描述:验证 watch_changes 方法的使用 # 预期结果:找到至少一处调用 watch_changes 的地方,通常在处理文件系统事件的代码中 # 测试:搜索 watch_changes 方法的调用 rg "plugin_driver\.watch_changes\(" --type rustLength of output: 325
crates/mako/src/dev.rs (2)
397-400
: 改进了错误处理机制这个修改通过使用
map_err()
替代unwrap()
来处理generate_end
方法的错误,提高了代码的健壮性。这种方式可以防止程序在遇到错误时崩溃,同时允许更优雅地处理和记录错误。
405-408
: 保持一致的错误处理方式这个修改与之前的改动类似,对
write_bundle
方法也采用了map_err()
来替代unwrap()
。这种一致的错误处理方式有助于提高代码的可读性和可维护性,同时也增强了整个rebuild
方法的健壮性。docs/config.md (3)
577-577
: 新增watchChanges
钩子函数的文档说明在
plugins
配置项中新增了watchChanges
钩子函数的说明。这个新增的钩子函数允许插件响应文件变化事件。
580-580
: LGTM:其他钩子函数的文档说明其他钩子函数的文档说明保持不变,内容清晰准确。
Also applies to: 581-581, 582-582, 583-583, 584-584, 585-585
Line range hint
1-594
: 文档更新总体评价本次文档更新新增了
watch
配置项和plugins.watchChanges
钩子函数的说明,这些添加反映了系统的新功能,对用户理解和使用这些新特性很有帮助。文档的整体结构和风格保持一致,内容清晰。建议:
- 为
watchChanges
钩子函数添加更详细的参数说明。- 在
watch
配置项中增加更多使用场景和示例。这些小的改进将进一步提高文档的可用性和完整性。
self.context.plugin_driver.watch_changes( | ||
&path.to_string_lossy(), | ||
"create", | ||
&self.context, | ||
)?; |
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
建议提取重复代码,减少冗余
在不同的 UpdateType
分支中,对 self.context.plugin_driver.watch_changes
的调用几乎相同,除了第二个参数不同("create"、"delete"、"update")。可以考虑将这些调用抽取出来,减少代码重复,提高代码的可读性和维护性。
可考虑如下修改:
match update_type {
- UpdateType::Add => {
- self.context.plugin_driver.watch_changes(
- &path.to_string_lossy(),
- "create",
- &self.context,
- )?;
- added.push(path);
- }
- UpdateType::Remove => {
- self.context.plugin_driver.watch_changes(
- &path.to_string_lossy(),
- "delete",
- &self.context,
- )?;
- removed.push(path);
- }
- UpdateType::Modify => {
- self.context.plugin_driver.watch_changes(
- &path.to_string_lossy(),
- "update",
- &self.context,
- )?;
- modified.push(path);
- }
+ UpdateType::Add => ("create", &mut added),
+ UpdateType::Remove => ("delete", &mut removed),
+ UpdateType::Modify => ("update", &mut modified),
};
+ self.context.plugin_driver.watch_changes(
+ &path.to_string_lossy(),
+ change_type,
+ &self.context,
+ )?;
+ vec_to_push.push(path);
这样可以减少重复的代码。
Also applies to: 197-201, 205-209
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1651 +/- ##
==========================================
- Coverage 55.59% 55.50% -0.10%
==========================================
Files 172 172
Lines 17429 17454 +25
==========================================
- Hits 9689 9687 -2
- Misses 7740 7767 +27 ☔ View full report in Codecov by Sentry. |
.
Summary by CodeRabbit
新功能
watch_changes
方法,允许插件监控文件变化。watch
,用户可指定在监控过程中忽略的路径。plugins
配置,新增watchChanges
属性以响应文件变化。文档