Avoid rc package deep imports#61
Conversation
|
@QDyanbing is attempting to deploy a commit to the React Component Team on Vercel. A member of the Team first needs to authorize it. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (1)
Walkthrough该PR统一@rc-component/util的导入为包级命名导入、升级依赖（含 React 18），并把测试框架从 Enzyme 完整迁移到 Changes导入路径统一与依赖升级
测试框架迁移
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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
src/Overflow.tsxESLint skipped: missing config or dependency (missing-dependency). The ESLint configuration references a package that is not available in the sandbox. tests/github.spec.tsxESLint skipped: the ESLint configuration for this file references a package that is not available in the sandbox. tests/wrapper.tsESLint skipped: the ESLint configuration for this file references a package that is not available in the sandbox. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request migrates the project's testing infrastructure from Enzyme to React Testing Library and upgrades React to version 18. Key changes include the removal of Enzyme-related dependencies, a complete rewrite of the test setup and utility wrappers to support RTL, and updating existing test suites to handle asynchronous operations and new assertion patterns. The Overflow component was also updated to provide index information to the renderItem callback and to use modernized imports from @rc-component/util. Review feedback highlights several areas for improvement in the new test wrapper, including the lack of support for React component selectors in the custom find method, potential issues with hardcoded class names when using custom prefixCls, and a performance optimization to avoid redundant DOM queries during initialization.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
tests/wrapper.ts (1)
224-232: 💤 Low value在循环中重复查询 DOM 可能导致意外行为。
initSize在每次循环迭代中调用两次queryOverflowItems()—— 一次获取长度，一次获取元素。如果 resize 触发重渲染导致 DOM 结构变化，可能会出现索引不匹配或跳过元素的情况。建议缓存初始元素列表，或改用 while 循环在每次迭代时重新获取当前第一个未处理的元素：
♻️ 建议的改进
async initSize(width: number, itemWidth: number) { await wrapper.triggerResize(width); - for (let index = 0; index < queryOverflowItems().length; index += 1) { - await triggerElementResize(queryOverflowItems()[index], itemWidth); + const items = queryOverflowItems(); + for (const item of items) { + await triggerElementResize(item, itemWidth); } return wrapper; },🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/wrapper.ts` around lines 224 - 232, The loop in initSize repeatedly calls queryOverflowItems() causing potential DOM-index mismatches after resize; fix by capturing the initial list into a local variable (e.g., const items = queryOverflowItems()) and iterate over that cached array when calling triggerElementResize(items[index], itemWidth) (or alternatively replace the for-loop with a while that re-queries and processes the first unhandled element), ensuring you reference initSize, queryOverflowItems, triggerElementResize and wrapper when making the change.tests/github.spec.tsx (1)
105-109: ⚡ Quick win建议使用更类型安全的查询方法。
当前使用
as any绕过了类型检查。考虑以下更好的方案：
- 使用 Testing Library 的查询方法（如
getByClassName）替代querySelector- 或者使用非空断言：
expect(container.querySelector('.rc-overflow-item-rest')!).toHaveStyle(...)♻️ 建议的改进方案
- (expect( - container.querySelector('.rc-overflow-item-rest'), - ) as any).toHaveStyle({ + expect( + container.querySelector('.rc-overflow-item-rest')!, + ).toHaveStyle({ opacity: 1, });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/github.spec.tsx` around lines 105 - 109, Replace the unsafe cast using "as any" when checking the element with selector '.rc-overflow-item-rest' in tests/github.spec.tsx: use a type-safe DOM access instead (e.g., call querySelector and assert it is non-null via a non-null assertion, or use container.getElementsByClassName('rc-overflow-item-rest') and pick the first element) and then call toHaveStyle on that element; update the assertion so it no longer relies on "as any" and ensures the element is present before asserting its style.tests/index.spec.tsx (1)
99-105: ⚖️ Poor tradeoff测试覆盖度不足：itemKey 用例仅断言渲染文本，未验证 React key 行为
tests/index.spec.tsx（99-116）中itemKey（string/function）两用例都只检查wrapper.findItems().text()，而 Reactkey不会出现在 DOM；tests/wrapper.ts也没有提供读取 Reactkey的能力。建议：
- 补充“重复 key 导致的 console.error/console.warn”这类间接校验（当前测试里未见相关拦截/断言，tests/setup.js 也未配置）。
- 或将用例名称改为反映实际验证内容（例如 “renders items correctly”）。
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/index.spec.tsx` around lines 99 - 105, The test titled 'string' only asserts rendered text and doesn't verify React key behavior; update the test to either (A) assert key-related side effects by spying on console.error/console.warn (use jest.spyOn(console, 'error'|'warn') and mockImplementation) and feed Overflow (renderItem/getData) a dataset with duplicate itemKey values to trigger the warning, or (B) rename the test to a descriptive title like "renders items correctly" to reflect that it only checks DOM text; adjust the other itemKey (function) test similarly and ensure tests/wrapper.ts or tests/setup.js includes any necessary console spies if you choose option A.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@tests/github.spec.tsx`:
- Around line 105-109: Replace the unsafe cast using "as any" when checking the
element with selector '.rc-overflow-item-rest' in tests/github.spec.tsx: use a
type-safe DOM access instead (e.g., call querySelector and assert it is non-null
via a non-null assertion, or use
container.getElementsByClassName('rc-overflow-item-rest') and pick the first
element) and then call toHaveStyle on that element; update the assertion so it
no longer relies on "as any" and ensures the element is present before asserting
its style.
In `@tests/index.spec.tsx`:
- Around line 99-105: The test titled 'string' only asserts rendered text and
doesn't verify React key behavior; update the test to either (A) assert
key-related side effects by spying on console.error/console.warn (use
jest.spyOn(console, 'error'|'warn') and mockImplementation) and feed Overflow
(renderItem/getData) a dataset with duplicate itemKey values to trigger the
warning, or (B) rename the test to a descriptive title like "renders items
correctly" to reflect that it only checks DOM text; adjust the other itemKey
(function) test similarly and ensure tests/wrapper.ts or tests/setup.js includes
any necessary console spies if you choose option A.
In `@tests/wrapper.ts`:
- Around line 224-232: The loop in initSize repeatedly calls
queryOverflowItems() causing potential DOM-index mismatches after resize; fix by
capturing the initial list into a local variable (e.g., const items =
queryOverflowItems()) and iterate over that cached array when calling
triggerElementResize(items[index], itemWidth) (or alternatively replace the
for-loop with a while that re-queries and processes the first unhandled
element), ensuring you reference initSize, queryOverflowItems,
triggerElementResize and wrapper when making the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 418c9460-567f-4aaf-be4a-66fd3b945b45
📒 Files selected for processing (14)
examples/fill-width.tsxjest.config.jspackage.jsonsrc/Overflow.tsxsrc/hooks/channelUpdate.tssrc/hooks/useEffectState.tsxtests/github.spec.tsxtests/index.spec.tsxtests/prefix.spec.tsxtests/raw.spec.tsxtests/responsive.spec.tsxtests/setup.jstests/ssr.spec.tsxtests/wrapper.ts
💤 Files with no reviewable changes (1)
- jest.config.js
|
Refs ant-design/ant-design#58115 antd 侧统一跟踪 rc 包 es/lib 深路径引用问题。 |
1 participant
背景
为配合 rc 包统一避免引用其他 rc 包的
es/lib构建产物内部路径，本次将 overflow 中相关引用调整为从包根入口导入公开 API。调整内容
@rc-component/father-plugin、@rc-component/resize-observer、@rc-component/util验证
npm run lint通过，存在既有 warning，无 errornpm test -- --runInBand通过，SSR 用例存在 ReactuseLayoutEffectconsole warningSummary by CodeRabbit
发布说明
Chores
@rc-component/util至 1.11.1Tests