-
-
Notifications
You must be signed in to change notification settings - Fork 214
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
perf: esm/cls browser targets #282
Conversation
📝 WalkthroughWalkthrough此次更改在 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 @@
## main #282 +/- ##
=======================================
Coverage 91.13% 91.13%
=======================================
Files 66 66
Lines 1422 1422
Branches 364 379 +15
=======================================
Hits 1296 1296
Misses 126 126 ☔ View full report in Codecov by Sentry. |
Bundle ReportBundle size has no change ✅ |
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: 0
🧹 Outside diff range and nitpick comments (2)
.fatherrc.ts (2)
22-24
: 建议:考虑更细粒度的浏览器目标配置目前的配置仅指定了 Chrome 80,建议考虑:
- 为不同的构建格式(ESM/CJS/UMD)设置差异化的目标
- 添加其他主流浏览器的支持
targets: { - chrome: 80, + chrome: 80, + firefox: 74, + safari: '13.1', + edge: 80 },
22-24
: 建议:监控包大小变化为了确保此更改达到预期效果,建议:
- 在合并前使用 Bundlephobia 验证包大小变化
- 建立包大小基准测试流程
- 考虑添加包大小限制检查
这将帮助团队及时发现和解决包大小问题。
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
.fatherrc.ts
(1 hunks)
🔇 Additional comments (1)
.fatherrc.ts (1)
22-24
: 浏览器目标配置的调整需要验证
将 targets
配置从 umd
部分移至根级别会影响所有构建格式(ESM、CJS、UMD)。这个改动可能会帮助优化包大小,但需要验证是否会影响到浏览器兼容性。
运行以下脚本来分析当前的包大小:
✅ Verification successful
浏览器目标配置调整是合理的优化
根据分析,这个改动是安全且合理的:
- package.json 中已设置
browserslist: ["defaults"]
作为基础兼容性配置 - 项目依赖中包含
browserslist-to-esbuild
,说明构建工具链已具备正确处理目标浏览器配置的能力 - 从 CHANGELOG 可见,项目此前就在持续优化包大小和浏览器兼容性策略
- Chrome 80 是一个合理的目标版本,可以支持现代 JavaScript 特性同时保持良好的兼容性
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: 分析包大小变化
# 检查 dist 目录下的文件大小
fd -e js -e css . dist/ --exec du -h {}
# 检查 package.json 中的浏览器目标配置
cat package.json | jq '.browserslist'
Length of output: 191
Script:
#!/bin/bash
# 检查项目中的其他构建配置文件
fd -e ts -e js -e json . -t f --exec grep -l "browserslist\|targets" {}
# 检查是否有 .browserslistrc 配置文件
fd "\.browserslistrc$"
# 检查 package.json 中的构建相关配置
cat package.json | jq '{dependencies,devDependencies,peerDependencies,browserslist}'
# 搜索代码库中可能的浏览器兼容性相关配置
rg -i "browser.*support|browser.*target|browserslist|chrome.*version" -g '!{node_modules,dist}/*'
Length of output: 7598
size-limit report 📦
|
Deploying ant-design-x with Cloudflare Pages
|
#234 里把 esm 和 cjs 的 targets 移除了,给 umd 加上了 targets,确实降低了 dist 的文件大小,但是把 bundlephobia 里的体积搞大了。
48.2kB
54.3kB
这里都加上。
Summary by CodeRabbit