Avoid rc package deep imports#656
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. |
Walkthrough此拉取请求执行了一次全面的测试框架迁移和类型系统重构：从 Enzyme 迁移至 React Testing Library，升级 React 16 到 18，并将关键类型定义从外部包嵌套路径本地化为 src/interface.ts 和相关模块，统一所有依赖导入为包公开入口。 ChangesEnzyme 迁移与类型系统重构
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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
jest.config.jsESLint skipped: missing config or dependency (missing-dependency). The ESLint configuration references a package that is not available in the sandbox. src/LegacyContext.tsxESLint skipped: the ESLint configuration for this file references a package that is not available in the sandbox. src/OptionList.tsxESLint 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.
|
There was a problem hiding this comment.
Code Review
This pull request primarily focuses on migrating the project's testing framework from Enzyme to React Testing Library and upgrading core dependencies, including React and React DOM to version 18. The changes involve removing Enzyme-related configurations and dependencies, refactoring extensive test suites to use the new testing library, and introducing a set of helper utilities to facilitate DOM-based testing. Additionally, the PR includes significant refactoring of internal imports and type definitions to improve code consistency. Feedback on the changes highlighted a type mismatch in src/OptionList.tsx, where the scrollTo property was defined as non-nullable despite being potentially undefined in the implementation; a suggestion was provided to align the type definition with the actual component behavior.
| interface RefOptionListProps { | ||
| onKeyDown: React.KeyboardEventHandler; | ||
| onKeyUp: React.KeyboardEventHandler; | ||
| scrollTo?: (args: unknown) => void; | ||
| } | ||
|
|
||
| type ScrollTo = NonNullable<React.ComponentRef<typeof Tree>['scrollTo']>; | ||
|
|
||
| type ReviseRefOptionListProps = Omit<RefOptionListProps, 'scrollTo'> & { scrollTo: ScrollTo }; |
There was a problem hiding this comment.
目前的类型定义中，ReviseRefOptionListProps 通过交叉类型将 scrollTo 设为了必填项，且使用了 NonNullable。但在组件实现中（第 278 行），scrollTo 的值来自于 treeRef.current?.scrollTo，在树未渲染（如数据为空）时可能为 undefined。这会导致类型定义与实际行为不符。建议直接在接口中定义可选的 scrollTo 类型，简化定义并保持与实现一致。
| interface RefOptionListProps { | |
| onKeyDown: React.KeyboardEventHandler; | |
| onKeyUp: React.KeyboardEventHandler; | |
| scrollTo?: (args: unknown) => void; | |
| } | |
| type ScrollTo = NonNullable<React.ComponentRef<typeof Tree>['scrollTo']>; | |
| type ReviseRefOptionListProps = Omit<RefOptionListProps, 'scrollTo'> & { scrollTo: ScrollTo }; | |
| interface RefOptionListProps { | |
| onKeyDown: React.KeyboardEventHandler; | |
| onKeyUp: React.KeyboardEventHandler; | |
| scrollTo?: React.ComponentRef<typeof Tree>['scrollTo']; | |
| } | |
| type ReviseRefOptionListProps = RefOptionListProps; |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@tests/util.tsx`:
- Around line 82-85: getVisibleTreeNodes incorrectly treats nodes with
aria-hidden="false" as hidden by using node.hasAttribute('aria-hidden'); update
the visibility check in getVisibleTreeNodes to only exclude nodes where
aria-hidden === "true" (e.g., use node.getAttribute('aria-hidden') !== 'true' or
change the query selector to
'.rc-tree-select-tree-treenode:not([aria-hidden="true"])') so that nodes with
aria-hidden="false" are considered visible.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6198b968-8cd7-432d-be77-9030b6fffa7e
⛔ Files ignored due to path filters (2)
tests/__snapshots__/Select.checkable.spec.tsx.snapis excluded by!**/*.snaptests/__snapshots__/Select.spec.tsx.snapis excluded by!**/*.snap
📒 Files selected for processing (25)
jest.config.jspackage.jsonsrc/LegacyContext.tsxsrc/OptionList.tsxsrc/TreeSelect.tsxsrc/TreeSelectContext.tssrc/hooks/useCheckedKeys.tssrc/hooks/useDataEntities.tssrc/interface.tssrc/utils/legacyUtil.tsxsrc/utils/strategyUtil.tssrc/utils/warningPropsUtil.tstests/Select.SearchInput.spec.jstests/Select.checkable.spec.tsxtests/Select.fieldNames.spec.tsxtests/Select.maxCount.spec.tsxtests/Select.multiple.spec.jstests/Select.props.spec.jstests/Select.spec.tsxtests/Select.tree.spec.jstests/Select.tree.treeExpandAction.spec.jstests/Select.warning.spec.jstests/setup.jstests/shared/focusTest.jstests/util.tsx
💤 Files with no reviewable changes (1)
- tests/setup.js
| export function getVisibleTreeNodes(element: HTMLElement = document.body) { | ||
| return Array.from(element.querySelectorAll('.rc-tree-select-tree-treenode')).filter( | ||
| node => !node.hasAttribute('aria-hidden'), | ||
| ); |
There was a problem hiding this comment.
getVisibleTreeNodes 对 aria-hidden="false" 会误判为不可见节点
Line 84 当前使用 hasAttribute('aria-hidden')，会把 aria-hidden="false" 也过滤掉，和其他用例中 :not([aria-hidden="true"]) 的可见性语义不一致，可能导致可见节点计数不稳定。
💡 建议修改
export function getVisibleTreeNodes(element: HTMLElement = document.body) {
return Array.from(element.querySelectorAll('.rc-tree-select-tree-treenode')).filter(
- node => !node.hasAttribute('aria-hidden'),
+ node => node.getAttribute('aria-hidden') !== 'true',
);
}🤖 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/util.tsx` around lines 82 - 85, getVisibleTreeNodes incorrectly treats
nodes with aria-hidden="false" as hidden by using
node.hasAttribute('aria-hidden'); update the visibility check in
getVisibleTreeNodes to only exclude nodes where aria-hidden === "true" (e.g.,
use node.getAttribute('aria-hidden') !== 'true' or change the query selector to
'.rc-tree-select-tree-treenode:not([aria-hidden="true"])') so that nodes with
aria-hidden="false" are considered visible.
背景
antd 侧需要移除对 rc 包
lib/es内部路径的依赖，统一改为通过 rc 包根入口使用公开 API。关联：ant-design/ant-design#58115
调整
@rc-component/select到~1.7.1，@rc-component/tree到~1.3.2@rc-component/select、@rc-component/tree的内部路径导入改为包根入口导入@rc-component/select新版本展示节点结构，更新相关测试断言和 snapshot验证
npm run compilenpm run lint（仅保留现有 react-hooks warning）npm test -- --runInBandSummary by CodeRabbit
发行说明
兼容性
改进