fix: node ignore should support node: prefixed only built-in modules #1721
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 (
|
![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 (5)
e2e/fixtures/config.platform.node/src/index.ts (1)
20-21: 导入语句格式建议建议在导入语句块中统一管理所有的
require调用，而不是分散在代码中。这样可以提高代码的可维护性和可读性。建议将代码重构为：
import { ENCODING } from './constants'; import fs1 from 'fs'; import fs2 from 'node:fs'; import { readFile } from 'fs/promises'; + require('http2'); + require('crypto'); + require('node:sqlite'); + fs1.readFileSync(__filename, ENCODING); fs2.readFileSync(__filename, ENCODING); readFile(__filename, { encoding: ENCODING }); console.log('dirname', __dirname); - require('http2'); - - try { - require('crypto') - hasCrypto = true - } catch { - hasCrypto = false - } - - require('node:sqlite'); + try { + hasCrypto = true + } catch { + hasCrypto = false + }e2e/fixtures/config.platform.node/expect.js (1)
38-38: 测试用例补充完善测试用例正确验证了
node:sqlite模块的保留，但建议同时添加负向测试用例，验证非node:前缀的sqlite模块是否被正确处理。建议添加如下测试：
assert(!content.includes(`require('sqlite');`), `should not keep require for non-node: prefixed sqlite`);crates/mako/src/features/node.rs (3)
24-29: 正则表达式模式优化建议当前实现使用了两个独立的正则表达式来匹配模块，这可能导致性能开销增加。建议合并为单个正则表达式以提高效率。
建议重构为：
- config.ignores.push(format!( - "^({})(/.+|$)", - Self::get_all_builtin_modules_besides_node_prefixed_only().join("|") - )); - config.ignores.push(format!( - "^node:({})(/.+|$)", - Self::get_all_builtin_modules().join("|") - )); + config.ignores.push(format!( + "^(?:node:)?({})(/.+|$)", + Self::get_all_builtin_modules().join("|") + ));
130-132: 内置模块列表维护建议
get_node_prefixed_only_builtin_modules方法中的模块列表是硬编码的，这可能导致维护困难。建议将模块列表移至配置文件中，便于后续更新和维护。建议创建一个配置文件来管理这些模块列表，例如：
// node_modules.config.json { "nodePrefixedOnlyModules": ["sqlite", "test"], "polyfillModules": [...], "emptyModules": [...] }
134-138: 代码复杂度优化建议
get_all_builtin_modules方法的实现可以更简洁。当前实现需要创建新的 Vec 并扩展，这会带来额外的内存分配。建议重构为：
fn get_all_builtin_modules() -> Vec<String> { let mut modules = Self::get_all_builtin_modules_besides_node_prefixed_only(); modules.extend(Self::get_node_prefixed_only_builtin_modules()); modules.sort(); modules.dedup(); modules }添加了
sort和dedup操作以确保模块列表的唯一性和排序。
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
crates/mako/src/features/node.rs(2 hunks)e2e/fixtures/config.platform.node/expect.js(1 hunks)e2e/fixtures/config.platform.node/src/index.ts(1 hunks)
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1721 +/- ##
==========================================
- Coverage 54.92% 54.89% -0.03%
==========================================
Files 178 178
Lines 17843 17871 +28
==========================================
+ Hits 9800 9811 +11
- Misses 8043 8060 +17 ☔ View full report in Codecov by Sentry. |
Re-implementation #1716
Summary by CodeRabbit
node:sqlite模块的支持，确保在转换输出中保留相关的require语句。require('node:sqlite');的存在。