[WIP] feat: keep unresolved nodejs require #1681
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Walkthrough该拉取请求对 Changes
Possibly related PRs
Suggested reviewers
Poem
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 ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1681 +/- ##
=======================================
Coverage 55.63% 55.64%
=======================================
Files 173 173
Lines 17528 17531 +3
=======================================
+ Hits 9752 9755 +3
Misses 7776 7776 ☔ 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 (2)
crates/mako/src/visitors/mako_require.rs (2)
52-54: 建议添加注释说明平台特定逻辑这段代码实现了在 Node 平台保留原生 require 的核心功能，建议添加注释说明这个特殊处理的目的。
建议添加如下注释：
+ // 在 Node 平台下保留原生 require，不进行替换 if let Platform::Node = self.context.config.platform { return; }
116-116: 建议增加 Node 平台场景的测试用例现有测试覆盖了基本的 require 转换场景，但缺少对新增的 Node 平台特殊处理的测试。
建议添加如下测试用例：
#[test] fn test_require_in_node_platform() { let js_code = r#"require("foo")"#; let mut test_utils = TestUtils::gen_js_ast(js_code); 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); }); // 验证在 Node 平台下 require 没有被替换 assert_eq!(test_utils.js_ast_to_code(), r#"require("foo");"#); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (12)
crates/mako/src/generate/transform.rs(1 hunks)crates/mako/src/visitors/mako_require.rs(3 hunks)packages/bundler-mako/package.json(1 hunks)packages/mako/npm/darwin-arm64/package.json(1 hunks)packages/mako/npm/darwin-x64/package.json(1 hunks)packages/mako/npm/linux-arm64-gnu/package.json(1 hunks)packages/mako/npm/linux-arm64-musl/package.json(1 hunks)packages/mako/npm/linux-x64-gnu/package.json(1 hunks)packages/mako/npm/linux-x64-musl/package.json(1 hunks)packages/mako/npm/win32-ia32-msvc/package.json(1 hunks)packages/mako/npm/win32-x64-msvc/package.json(1 hunks)packages/mako/package.json(2 hunks)
✅ Files skipped from review due to trivial changes (9)
- packages/bundler-mako/package.json
- packages/mako/npm/darwin-arm64/package.json
- packages/mako/npm/darwin-x64/package.json
- packages/mako/npm/linux-arm64-gnu/package.json
- packages/mako/npm/linux-arm64-musl/package.json
- packages/mako/npm/linux-x64-gnu/package.json
- packages/mako/npm/linux-x64-musl/package.json
- packages/mako/npm/win32-ia32-msvc/package.json
- packages/mako/npm/win32-x64-msvc/package.json
🔇 Additional comments (4)
packages/mako/package.json (2)
3-3: 注意：这是一个金丝雀（预发布）版本
当前更改将版本从稳定版 0.9.4 更新为预发布版本 0.9.5-canary.20241107.2。由于这是一个 WIP PR，使用金丝雀版本是合适的，但请确保在最终发布前进行充分测试。
78-85: 验证所有平台构建包的可用性
所有可选依赖已更新到新版本 0.9.5-canary.20241107.2。
让我们验证这些包是否都已经发布到 npm：
✅ Verification successful
所有平台构建包已成功发布且可用
验证结果显示所有 8 个平台的构建包都已成功发布到 npm 仓库，且版本号均为 0.9.5-canary.20241107.2：
- @umijs/mako-darwin-arm64
- @umijs/mako-linux-arm64-gnu
- @umijs/mako-linux-arm64-musl
- @umijs/mako-win32-ia32-msvc
- @umijs/mako-darwin-x64
- @umijs/mako-win32-x64-msvc
- @umijs/mako-linux-x64-gnu
- @umijs/mako-linux-x64-musl
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# 描述：验证所有平台构建包是否已发布到 npm
echo "检查以下包的可用性："
for pkg in \
"@umijs/mako-darwin-arm64@0.9.5-canary.20241107.2" \
"@umijs/mako-linux-arm64-gnu@0.9.5-canary.20241107.2" \
"@umijs/mako-linux-arm64-musl@0.9.5-canary.20241107.2" \
"@umijs/mako-win32-ia32-msvc@0.9.5-canary.20241107.2" \
"@umijs/mako-darwin-x64@0.9.5-canary.20241107.2" \
"@umijs/mako-win32-x64-msvc@0.9.5-canary.20241107.2" \
"@umijs/mako-linux-x64-gnu@0.9.5-canary.20241107.2" \
"@umijs/mako-linux-x64-musl@0.9.5-canary.20241107.2"
do
npm view $pkg version || echo "包 $pkg 不存在"
done
Length of output: 4570
crates/mako/src/visitors/mako_require.rs (1)
1-2: 结构变更符合设计要求！
新增的 Arc<Context> 字段能够安全地共享上下文信息，这对于处理 Node.js require 场景是必要的。使用 Arc 确保了线程安全性，同时避免了重复克隆上下文数据。
Also applies to: 9-10, 17-17
crates/mako/src/generate/transform.rs (1)
250-250: 代码变更符合预期且实现正确
这个改动通过向 MakoRequire 添加 context 来支持保留未解析的 nodejs require，这与 PR 的目标相符。使用 Arc::clone 来共享 context 是正确的做法，因为它只会增加引用计数而不会深度克隆数据。
让我们验证一下 MakoRequire 结构体的定义是否正确更新：
xusd320
force-pushed
the
feat/keep-unresolved-nodejs-require
branch
from
d4ca903 to
6cd0fe8
Compare
xusd320
force-pushed
the
feat/keep-unresolved-nodejs-require
branch
from
6cd0fe8 to
eacee04
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
faas 场景的特殊需求，对于不能分析出依赖的 require, 保留 nodejs 原生 require。
Summary by CodeRabbit
新特性
MakoRequire结构，新增context字段，以便在 JavaScript 转换过程中利用上下文信息。visit_mut_ident方法，以支持平台特定的行为，确保在 Node 平台下不替换标识符。版本更新
0.9.5-canary.20241107.2。