-
Notifications
You must be signed in to change notification settings - Fork 69
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
refactor: all flattenToken need hashed #204
Conversation
📝 WalkthroughWalkthrough本次更改主要涉及 Changes
Assessment against linked issues
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 eslint
warning [email protected]: This version is no longer supported. Please see https://eslint.org/version-support for other options. 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 (
|
🎊 PR Preview 2bfc425 has been successfully built and deployed to https://ant-design-cssinjs-preview-pr-204.surge.sh 🕐 Build time: 91.206s 🤖 By surge-preview |
if (hashed) { | ||
str = hash(str); | ||
} | ||
str = hash(str); |
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.
上面的 #48386 应该是跷跷板问题吧,hash 不缩短,拼接就会很长🤔
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.
额,48386 指的是什么?
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.
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.
应该不是这个……
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 (3)
src/util/index.ts (2)
56-56
: 建议添加更详细的注释说明哈希的必要性当前注释仅提到了样式标签长度问题(#48386),建议补充说明这个改动对解决内存泄漏的帮助。
- // https://github.com/ant-design/ant-design/issues/48386 - // Should hash the string to avoid style tag name too long + // 1. Hash the string to avoid style tag name too long (#48386) + // 2. Consistent hashing helps prevent memory leaks by ensuring token identity
Line range hint
38-60
: 缓存策略的实现值得肯定使用 WeakMap 作为缓存机制是个很好的选择:
- 允许未使用的令牌被垃圾回收,防止内存泄漏
- 通过分离不同用途的缓存(resultCache 和 flattenTokenCache)保持了关注点分离
这种实现方式很好地平衡了性能和内存管理。
tests/index.spec.tsx (1)
205-205
: 建议避免硬编码哈希值在测试中直接比较硬编码的哈希值可能会导致测试在哈希算法或输入更改时失败。建议使用动态计算的哈希值或使用更健壮的方式进行验证。
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
src/util/index.ts
(3 hunks)tests/index.spec.tsx
(1 hunks)tests/util.spec.tsx
(1 hunks)
🔇 Additional comments (3)
tests/util.spec.tsx (1)
128-128
: 测试用例已正确更新以匹配新的哈希实现
测试用例已更新以反映 flattenToken
函数的新行为,现在始终返回哈希值。这与重构目标保持一致。
运行以下脚本以验证是否还有其他使用 flattenToken
的测试用例需要更新:
✅ Verification successful
测试更新验证完成,无需额外修改
搜索结果显示:
flattenToken
函数仅在tests/util.spec.tsx
中被使用一次- 这正是我们已经审查的测试用例
- 没有发现其他需要更新的测试用例
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# 描述:查找可能需要更新的其他测试用例
# 搜索所有测试文件中的 flattenToken 调用
rg -t tsx -t ts "flattenToken\(" "tests/"
# 搜索可能包含旧输出格式的断言
rg -t tsx -t ts "expect\(.*flattenToken.*\)" "tests/"
Length of output: 151
Script:
#!/bin/bash
# 使用更通用的文件搜索方式
# 使用 fd 查找所有测试文件
fd "\.(test|spec)\.(ts|tsx|js|jsx)$" tests/
# 使用 ripgrep 搜索 flattenToken 的调用,不指定文件类型
rg "flattenToken\(" tests/
# 搜索包含 flattenToken 的断言语句
rg "expect.*flattenToken" tests/
Length of output: 495
src/util/index.ts (2)
68-68
: 令牌键生成逻辑的更新是正确的
移除了不必要的 hashed
参数,同时保持了基于盐值的哈希安全模型。这个改动与 flattenToken
的更改保持一致。
Line range hint 38-48
: 强制哈希处理所有令牌的更改是合理的
这个更改通过移除 hashed
参数并始终进行哈希处理,使令牌处理更加一致和可预测。这有助于解决内存泄漏问题。
不过建议验证一下递归实现的安全性。
✅ Verification successful
递归实现是安全的,无需担心循环引用问题
基于代码分析结果,我可以确认 flattenToken
函数的实现是安全的,原因如下:
- 使用
WeakMap
(flattenTokenCache
) 进行缓存,可以有效防止重复处理相同的令牌对象 - 测试用例 (
tests/util.spec.tsx
) 验证了缓存机制的有效性,即使调用 10000 次,getter 也只被调用一次 - 递归实现时:
- 只处理对象的可枚举属性
- 对
Theme
实例做了特殊处理,直接使用其id
属性 - 使用哈希函数处理最终字符串,确保输出长度可控
这些机制共同确保了函数不会陷入无限递归,同时能高效处理嵌套的令牌对象。
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# 描述:检查是否存在可能导致无限递归的循环引用令牌
# 搜索包含循环引用的测试用例
rg -l "const.*=.*{.*:.*}" tests/
Length of output: 145
Script:
#!/bin/bash
# 搜索可能的循环引用模式
rg -A 5 "const\s+\w+\s*=\s*\{" tests/
# 检查测试文件中的 flattenToken 相关测试用例
rg -A 10 "flattenToken" tests/
# 检查源代码中是否有防止循环引用的机制
ast-grep --pattern 'function flattenToken($_) {
$$$
}'
Length of output: 4974
当前的 PR 修复方式和之前有人打补丁的行为一样,只是解决 react hook 的 依赖大小 。 |
修复了 ant-design/ant-design#49885 么? |
同问, antd依赖也没update |
fix #197
fix #188
close #198
useGlobalCache
需要走一遍useMemo
cache,deps 由于会被 React Fiber cache 导致内存占用。这里拿时间换一点空间出来,从而节省巨型页面中的内存占用情况。Summary by CodeRabbit
新功能
错误修复
文档
flattenToken
函数的行为变化,确保缓存机制正常工作。