-
Notifications
You must be signed in to change notification settings - Fork 85
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 unplugin context #1728
Conversation
概述代码变更解析主要变更概述本次拉取请求引入了对 Mako 构建工具的重大改进,主要集中在插件上下文和钩子函数的增强。核心变更包括:
变更详情代码走查本次拉取请求引入了对 Mako 构建工具插件系统的显著改进。主要变更包括在多个文件中添加 变更
序列图sequenceDiagram
participant Plugin
participant PluginContext
participant BuildSystem
Plugin->>PluginContext: 创建上下文
Plugin->>BuildSystem: 调用钩子函数
BuildSystem->>PluginContext: 传递上下文信息
PluginContext-->>Plugin: 提供上下文方法
可能相关的 PR
建议的审阅者
诗歌庆祝
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command 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 (
|
WalkthroughThis pull request introduces support for unplugin context by adding a Changes
|
#[napi] | ||
impl PluginContext { | ||
#[napi] | ||
pub fn warn(&self, msg: String) { |
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.
Ensure that the warn
method in PluginContext
handles logging in a thread-safe manner if used in concurrent environments.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1728 +/- ##
==========================================
- Coverage 54.92% 54.80% -0.12%
==========================================
Files 180 180
Lines 17931 18045 +114
==========================================
+ Hits 9848 9890 +42
- Misses 8083 8155 +72 ☔ View full report in Codecov by Sentry. |
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
🧹 Nitpick comments (20)
e2e/fixtures/plugins.unplugin/plugins.config.js (1)
4-12
: 使用replace.raw
替换字符串。
这里将FOOOO
替换为"fooooooo"
,可以直接满足自定义文本替换需求。确认是否需要更多的占位符或更多的替换规则以满足其他场景。crates/binding/src/js_hook.rs (1)
88-98
: 为TsFnHooks
引入新的ThreadsafeFunction
签名这些字段大幅增加了对
PluginContext
的支持,使得异步回调可以获得更多上下文信息。有几点需要关注:
- 调用时需确保传入的
PluginContext
合理地被共享或克隆,避免竞争条件。- 返回类型中带有
Option<...>
时,应在 TypeScript 侧或 Rust 端做空值判定,确保流程安全。此设计整体看来可读性尚可,若后续功能继续扩展,需进一步整理或抽象避免钩子过多变得臃肿。
crates/binding/src/js_plugin.rs (6)
9-9
: 多余或缺少注释的确认在第 9 行新引入了
use napi_derive::napi;
。请确认是否需要在文件顶部添加相关注释,说明#[napi]
宏的使用场景。若已有注释可忽略。
146-155
:generate_end
与build_end
的合并调用此处先后调用了
generate_end
和build_end
,实现对旧接口与新接口的兼容;建议在日后可考虑统一命名或合并逻辑,减少重复代码和认知成本。
158-160
: 同时调用build_end
钩子
build_end
钩子现在与generate_end
写在一起,调用顺序上可能引发混淆。若逻辑需要,保持即可;若可并至同一流程则可避免重复回调。
165-170
:watch_changes
增加上下文以处理文件变动可进一步考虑对
event
值的枚举化处理,以减少字符串硬编码和潜在错误。
189-204
:before_write_fs
回调处理在写文件前调用
_on_generate_file
,可在此处做额外的权限校验或路径规则过滤,防止不安全的文件写入。
234-240
:transform
中返回结果的容错处理当
hook.call(...)
返回None
时,目前逻辑会跳过处理;若要收集所有处理结果或错误原因,可在返回None
时适当记录日志,便于排查。packages/mako/src/index.ts (3)
138-138
: 判断并写入mako.config.json
此处基于
process.env.DUMP_MAKO_CONFIG
条件触发写入配置文件,可在后续考虑增加更灵活的命令行选项或配置项,避免对环境变量的过度依赖。
189-190
: 为插件添加上下文包装在处理插件之前(行 189-190),即将要为插件的各个方法注入上下文。可在此处记录日志,方便调试插件注入。
301-301
: 清理无用或重复的空行若无特殊需求,可删除多余的空行保持文件整洁。
e2e/fixtures/plugins.context/plugins.config.js (1)
3-7
: 建议避免忽略.endsWith('.hoo')
的判断结果
在loadInclude(path)
中仅调用path.endsWith('.hoo')
却没有使用其返回值。若想根据文件后缀进行过滤、可将逻辑改为return path.endsWith('.hoo')
来确保只处理.hoo
文件。scripts/mako.js (1)
21-24
: 在捕获错误后可考虑输出更多上下文
console.error(e)
可帮助定位错误,但若要诊断更复杂问题,考虑输出堆栈e.stack
或更多上下文信息,以便快速排查。packages/mako/binding.d.ts (1)
273-277
: 确认emitFile
的参数是否满足需求
emitFile(originPath, outputPath)
参数可用于输出文件,但若需支持多种文件类型或内容自定义,考虑增加相应的入参(如source
或fileType
)。crates/mako/src/plugins/bundless_compiler.rs (1)
133-133
: 【避免直接使用 unwrap】
此处调用before_write_fs
后紧跟unwrap()
可能在发生错误时直接引发 panic,建议使用更安全的错误处理方式(如?
或自定义错误处理),以免意外中断程序流程。- .before_write_fs(&to, content.as_ref(), &self.context) - .unwrap(); + if let Err(e) = self.context + .plugin_driver + .before_write_fs(&to, content.as_ref(), &self.context) + { + // 在此处理错误,避免导致程序崩溃 + eprintln!("before_write_fs 失败: {:?}", e); + }docs/config.zh-CN.md (2)
585-590
: 【英文表达与标点改进】
这里的文档列出了钩子里可用的内置方法,整体较清晰,但部分英文描述如 “emit a error” 等细微处可改进为更准确的表达(e.g. “emit an error”)。
591-591
: 【用词建议】
“Plugins 兼容 [unplugin]” 一句中,若想表意更自然,可在中文上下文直接沿用 “插件”,如“这些插件可与 [unplugin] 生态兼容”。docs/config.md (3)
585-585
: 【用语统一性优化】
“this methods” 建议改为 “these methods”,以保证英文描述的语法一致性。
587-592
: 【文档可读性提升】
列举的方法示例内容充足,但英文表述里出现 “emit a error”等小细节,可改为 “emit an error”,更符合常规用法。🧰 Tools
🪛 LanguageTool
[uncategorized] ~587-~587: Loose punctuation mark.
Context: ... string, source: string | Uint8Array }), emit a file -
this.warn(message: strin...(UNLIKELY_OPENING_PUNCTUATION)
[misspelling] ~589-~589: Use “an” instead of ‘a’ if the following word starts with a vowel sound, e.g. ‘an article’, ‘an hour’.
Context: ...g -this.error(message: string)
, emit a error -this.parse(code: string)
, par...(EN_A_VS_AN)
593-593
: 【语法小建议】
“Plugins is compatible with...” 建议改为 “Plugins are compatible with...”,以符合语言习惯。🧰 Tools
🪛 LanguageTool
[grammar] ~593-~593: Did you mean “are” or “were”?
Context: ...file (CURRENTLY NOT SUPPORTED) Plugins is compatible with [unplugin](https://unpl...(SENT_START_NNS_IS)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
e2e/fixtures/plugins/node_modules/plugin/index.js
is excluded by!**/node_modules/**
pnpm-lock.yaml
is excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (21)
crates/binding/src/js_hook.rs
(2 hunks)crates/binding/src/js_plugin.rs
(8 hunks)crates/mako/src/plugin.rs
(2 hunks)crates/mako/src/plugins/bundless_compiler.rs
(1 hunks)docs/config.md
(1 hunks)docs/config.zh-CN.md
(1 hunks)e2e/fixtures/plugins.context/expect.js
(1 hunks)e2e/fixtures/plugins.context/mako.config.json
(1 hunks)e2e/fixtures/plugins.context/plugin.js
(1 hunks)e2e/fixtures/plugins.context/plugins.config.js
(1 hunks)e2e/fixtures/plugins.context/src/foo.hoo
(1 hunks)e2e/fixtures/plugins.context/src/index.tsx
(1 hunks)e2e/fixtures/plugins.unplugin/expect.js
(1 hunks)e2e/fixtures/plugins.unplugin/mako.config.json
(1 hunks)e2e/fixtures/plugins.unplugin/plugins.config.js
(1 hunks)e2e/fixtures/plugins.unplugin/src/index.tsx
(1 hunks)package.json
(2 hunks)packages/mako/binding.d.ts
(1 hunks)packages/mako/binding.js
(1 hunks)packages/mako/src/index.ts
(4 hunks)scripts/mako.js
(1 hunks)
✅ Files skipped from review due to trivial changes (4)
- e2e/fixtures/plugins.context/src/index.tsx
- e2e/fixtures/plugins.context/src/foo.hoo
- e2e/fixtures/plugins.context/mako.config.json
- e2e/fixtures/plugins.unplugin/mako.config.json
🧰 Additional context used
🪛 LanguageTool
docs/config.md
[uncategorized] ~587-~587: Loose punctuation mark.
Context: ... string, source: string | Uint8Array }), emit a file -
this.warn(message: strin...
(UNLIKELY_OPENING_PUNCTUATION)
[misspelling] ~589-~589: Use “an” instead of ‘a’ if the following word starts with a vowel sound, e.g. ‘an article’, ‘an hour’.
Context: ...g - this.error(message: string)
, emit a error - this.parse(code: string)
, par...
(EN_A_VS_AN)
[grammar] ~593-~593: Did you mean “are” or “were”?
Context: ...file (CURRENTLY NOT SUPPORTED) Plugins is compatible with [unplugin](https://unpl...
(SENT_START_NNS_IS)
🔇 Additional comments (27)
e2e/fixtures/plugins.unplugin/src/index.tsx (3)
1-2
: 在这里导入图标库和 React 看起来没问题。
这些导入语句符合项目需求,但请确保对应的依赖在项目中均已正确安装和配置,避免在构建或运行时出现找不到模块的错误。
4-4
: console.log(FOOOO)
中变量未在代码内定义。
此处直接调用 FOOOO
变量会导致运行错误,除非它在编译或替换阶段由其他插件注入并替换为实际字符串值。如果确实依赖插件注入,这样写可以保留;但如果并没有在最终代码里替换,可能会引发报错。
5-5
: 默认导出的函数式组件符合 React 习惯用法。
此处直接使用 <IconAccountBox />
返回一个图标组件,写法简洁易读。可根据实际需要补充更多逻辑或属性。
e2e/fixtures/plugins.unplugin/plugins.config.js (2)
1-3
: 导入 unplugin-replace
和 unplugin-icons
依赖正常。
请确保在 package.json
中已正确添加对这些依赖的版本号和说明,以保证安装和运行一致。
13-18
: 使用 unplugin-icons
处理图标。
启用了 jsx: 'react'
并关闭自动安装 (autoInstall: false
) 合理;如果需要自动增量加载图标,可考虑开启 autoInstall
。
e2e/fixtures/plugins.unplugin/expect.js (1)
1-8
: 测试脚本对编译结果进行断言。
content.includes('fooooooo')
:验证字符串替换是否成功content.includes('fill: "currentColor",')
:验证图标生成是否包含正确填充属性
测试内容覆盖较好,但可考虑增加更多断言以验证其他核心用例。
package.json (2)
28-28
: 新增 @iconify-json/mdi 依赖。
在开发环境中安装该依赖可使项目支持 MDI 图标集,但请确认是否需要在生产环境中也进行相应配置。
59-60
: 添加 unplugin-icons
和 unplugin-replace
。
它们与本次提交的插件配置相呼应,依赖声明合理。若项目计划进一步扩展更多 icon 集或字符串替换功能,请同步更新文档。
crates/binding/src/js_hook.rs (2)
6-6
: 引入 PluginContext
的用途检查
看起来在这里新引入了 use crate::js_plugin::PluginContext;
。建议确认该引入在后续字段或方法中都有良好且一致的使用场景,避免多余依赖或反复修改。
85-86
: 新增 ResolveIdFuncParams
类型
此处将 PluginContext
与额外参数组合成新的参数类型 ResolveIdFuncParams
。在调用时,请确保对元组解构或使用方式一致,避免解构顺序混淆。
crates/binding/src/js_plugin.rs (6)
31-52
: PluginContext
结构体与实现:线程安全与整合建议
已定义了 PluginContext
并实现了 warn
、error
、emit_file
等方法,方便在钩子回调中使用。以下建议可考虑:
- 为
warn
、error
添加更灵活的日志等级或允许自定义日志输出,避免后期统一日志系统时出现割裂。 - 如果多线程环境下可能同时调用
warn
等方法,需再三确认println!
使用符合并发安全需求(这与之前的评论类似,如需进一步完善可考虑在高并发环境下改用同步锁或日志库)。
69-73
: build_start
钩子调用上下文
在 build_start
中将 PluginContext
克隆传递给 hook.call
。这通常是正确的方式,但要确保在上下文中存放的资源,如 Arc<Context>
,不会产生循环引用或泄漏。
78-95
: load
方法中添加了额外 load_include
判断
逻辑上先检查 load_include
再执行 load
,若返回 false
则直接跳过后续处理。这样做能减少不必要的加载,但要确保对 Some(false)
的场景在 TypeScript 侧也做好对应的注解与回退机制。
112-118
: resolve_id
钩子的上下文传入
在这里通过元组 (PluginContext, String, String, ResolveIdParams)
拆解,建议在调用处明确区分 source
、importer
等参数的含义,减少命名出错风险。整体逻辑可行,无明显问题。
180-184
: write_bundle
新增上下文
与其他钩子调用逻辑类似,确认 context
内部共享状态不会对输出打包流程产生阻塞或死锁。
213-222
: transform_include
的布尔判定
若此处返回 Some(false)
,则直接跳过后续的 transform
操作。在多插件场景中,要确保各插件对 transform_include
的理解一致,否则可能出现意外跳过。
packages/mako/src/index.ts (2)
2-2
: 引入 os
模块
新增 import os from 'os';
后主要用于生成临时文件等操作,请确保在不需要时不要产生冗余依赖。
321-335
: adapterResult
工具函数
在此函数中对字符串或对象进行兼容处理,非常实用。若后续想支持更多类型(如 .css
或 .vue
),建议将 type
的处理扩展为枚举或可配置方式,以方便维护。
e2e/fixtures/plugins.context/plugin.js (1)
1-6
: 新建插件文件:对后缀名 .hoo
的基本检查
此处只对传入的 path
判定后缀 .hoo
,但未返回任何内容或进行错误处理。若预期只做简单过滤,可在注释中说明用途;若后期要做更多处理,比如返回自定义结果,需在逻辑中补充对应实现。
e2e/fixtures/plugins.context/expect.js (1)
1-7
: 测试断言逻辑
测试使用了 parseBuildResult
并检查了 files['test.txt']
是否等于 'test'
,逻辑相对简单明了。可在后续根据更多测试场景,验证文件内容、插件加载正确性等,确保功能完整。
e2e/fixtures/plugins.context/plugins.config.js (1)
8-25
: 注意 this.error()
会终止构建流程
在 load(path)
方法中,如果 path.endsWith('.hoo')
就会调用 this.error(...)
,这通常会抛出错误并终止构建流程。若预期只是警告或提示,建议改为更合适的日志级别或采用条件判断来避免影响正常打包过程。
scripts/mako.js (2)
10-10
: 确认 --watch
选项处理逻辑
这里使用 process.argv.includes('--watch')
检查是否处于 watch 模式十分直观且简洁,适用于 Node.js 12+ 环境。
16-20
: 在 watch 模式下保持进程存活
在 .then()
中对 !watch
退出进程的处理逻辑清晰直接;当启用 watch 时进程将不退出,从而持续监听文件变化。实现合理。
packages/mako/binding.js (1)
307-309
: 导出 PluginContext
以支持更多上下文能力
将 PluginContext
随同 build
一并导出,有助于插件在外部调用时灵活使用上下文方法,符合插件体系的扩展需求。
crates/mako/src/plugin.rs (2)
187-192
: 【新增参数设计合理】
新增的 before_write_fs
方法签名包含了 _context
参数,有助于在文件写入之前利用上下文信息完成更多逻辑。当前实现简单且无明显问题。
430-433
: 【确认所有插件实现已同步更新】
在 PluginDriver
中为 before_write_fs
新增了 context
参数。确认所有自定义插件的实现也同步更新,否则可能出现编译或运行时错误。
docs/config.zh-CN.md (1)
583-583
: 【文档表述无明显问题】
此处介绍针对 hook 函数可用的方法,内容清晰,无需改动。
}, | ||
// https://rollupjs.org/plugin-development/#this-addwatchfile | ||
addWatchFile(_file: string) { | ||
throw new Error('addWatchFile is not supported'); | ||
}, | ||
// https://rollupjs.org/plugin-development/#this-emitfile | ||
// only support asset type | ||
emitFile(file: { | ||
type: 'asset' | 'chunk' | 'prebuilt-chunk'; | ||
name?: string; | ||
fileName?: string; | ||
source?: string | Uint8Array; | ||
}) { | ||
if (file.type !== 'asset') { | ||
throw new Error('emitFile only support asset type'); | ||
} | ||
if (file.name && !file.fileName) { | ||
throw new Error( | ||
'name in emitFile is not supported yet, please supply fileName instead', | ||
); | ||
} | ||
// Since assets_info in mako is a <origin_path, output_path> map, | ||
// we need to generate a tmp file to store the content, and then emit it | ||
// TODO: we should use a better way to handle this | ||
const tmpFile = path.join( | ||
os.tmpdir(), | ||
Math.random().toString(36).substring(2, 15), | ||
); | ||
fs.writeFileSync(tmpFile, file.source!); | ||
context.emitFile(tmpFile, file.fileName!); | ||
}, | ||
warn( | ||
message: | ||
| string | ||
| { message: string; pluginCode?: string; meta?: string }, | ||
) { | ||
if (typeof message === 'object') { | ||
const msg = [ | ||
message.message, | ||
message.pluginCode | ||
? `pluginCode: ${message.pluginCode}` | ||
: '', | ||
message.meta ? `meta: ${message.meta}` : '', | ||
] | ||
.filter(Boolean) | ||
.join('\n'); | ||
context.warn(msg); | ||
} else { | ||
context.warn(message); | ||
} | ||
}, | ||
error( | ||
message: | ||
| string | ||
| { message: string; pluginCode?: string; meta?: string }, | ||
) { | ||
if (typeof message === 'object') { | ||
const msg = [ | ||
message.message, | ||
message.pluginCode | ||
? `pluginCode: ${message.pluginCode}` | ||
: '', | ||
message.meta ? `meta: ${message.meta}` : '', | ||
] | ||
.filter(Boolean) | ||
.join('\n'); | ||
context.error(msg); | ||
} else { | ||
context.error(message); | ||
} | ||
}, | ||
}, | ||
[...args], | ||
); | ||
// adapter mako hooks for unplugin | ||
if (key === 'load' || key === 'transform') { | ||
// if result is null, return the original code | ||
if (result === null) { | ||
result = args[0]; | ||
} | ||
const isPromise = typeof result === 'object' && result.then; | ||
if (isPromise) { | ||
result = result.then((result: any) => adapterResult(result)); | ||
} else { | ||
result = adapterResult(result); | ||
} | ||
} | ||
if (key === 'resolveId') { | ||
if (typeof result === 'string') { | ||
result = { | ||
id: result, | ||
external: false, | ||
}; | ||
} | ||
} | ||
return result; | ||
}; | ||
} | ||
}); | ||
}); |
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
插件钩子包装以适配 unplugin
风格
此段代码核心在于给每个插件的函数包装上下文对象并兼容 unplugin 接口风格,主要建议:
- 对
emitFile
等方法限制仅支持asset
类型的逻辑合理,但应明确告诉用户如何扩展其他类型。 warn
和error
中对message
的结构化处理很好,能附带pluginCode
与meta
。若后期需要更复杂的报错机制,可做更深入封装。transform
等方法若返回Promise
,利用adapterResult
进一步做结构适配,这种设计有利于保持统一的返回值类型;请注意可能出现的异步错误处理问题,建议捕获并记录。
Close #1238