feat: Remove findDOMNode from rc-css-motion #59
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
概述遍历在 变更
诗歌
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 (
|
![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
🧹 Nitpick comments (1)
src/CSSMotion.tsx (1)
143-145: 建议添加空值检查以提高健壮性。此处直接根据
nodeRef.current是否是HTMLElement来决定返回值，做法简洁且符合现代 React 不建议使用findDOMNode的实践。不过，如果nodeRef.current和wrapperNodeRef.current均为 null 或未定义，可能会导致后续引用报错。建议在返回前做好空值检查，以便在极端场景下能优雅处理。可考虑如下改进：
function getDomElement() { + if (!nodeRef.current && !wrapperNodeRef.current) { + return null; + } return nodeRef.current instanceof HTMLElement ? nodeRef.current : wrapperNodeRef.current; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/CSSMotion.tsx(1 hunks)tests/CSSMotion.spec.tsx(1 hunks)
🔇 Additional comments (2)
tests/CSSMotion.spec.tsx (2)
833-843: 测试逻辑清晰，符合无 ref 传递场景的需求。该测试通过直接渲染一个普通的
div并检查其是否被成功挂载，说明移除findDOMNode不影响正常 DOM 渲染。整体实现简单明了，保障了在严格模式下的正确性。
846-854: 测试逻辑恰当，验证了传递内部 ref 的正确性。该用例确保当我们在子组件内部将
ref传给真实 DOM 时，DOM 能够成功挂载并被检测到，进一步保证了移除findDOMNode后的可用性。
|
ci挂了 |
我再改下 |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
tests/CSSMotion.spec.tsx (4)
298-301: 建议添加 cleanup 逻辑虽然 mock
addEventListener是必要的，但建议在afterAll中清理这个 spy。beforeAll(() => { jest.spyOn(document, 'addEventListener').mockImplementation(() => {}); }); +afterAll(() => { + jest.restoreAllMocks(); +});
339-339: 简化 ref 转发实现当前的注释和实现可以更简洁。
- return <div ref={ref} {...props} />; // 使用 forwardRef 正确转发 ref + return <div {...props} ref={ref} />;
343-364: 优化测试用例结构和断言测试用例结构良好，但可以进一步改进：
- 移除冗余的注释
- 使用更具描述性的测试名称
- 添加更多边缘情况的测试
- it('FC with ref', () => { + it('should correctly forward ref in functional component', () => { const ref = React.createRef<HTMLDivElement>(); - // 使用 act 包裹渲染过程，确保状态更新 - let container; + let container: HTMLElement; act(() => { - // 仅在 act 内进行渲染，以确保是同步的 const { container: renderedContainer } = render( - <ForwardedComponent ref={ref} visible={true} />, // visible 为布尔值 + <ForwardedComponent ref={ref} visible={true} />, ); - container = renderedContainer; // 获取容器 + container = renderedContainer; }); - // 获取 div 元素，确保其正确渲染 const div = container.querySelector('div'); - // 确保 div 元素渲染 expect(div).toBeTruthy(); - // 确保 ref 被正确绑定到 div 元素 expect(ref.current).toBe(div); + + // Test visibility toggle + act(() => { + render(<ForwardedComponent ref={ref} visible={false} />, { container }); + }); + expect(div.style.display).toBe('none'); });
855-876: 完善严格模式测试覆盖测试用例覆盖了基本场景，但建议添加更多边缘情况的测试：
- 组件卸载时的行为
- 多次渲染时的行为
- 错误处理场景
it('renders correctly when no ref is passed', () => { const Div = () => <div />; const { container } = render( <CSSMotion motionName="transition" visible> {(props, ref) => <Div {...props} ref={ref} />} </CSSMotion>, ); - // 检查 DOM 是否渲染出来 expect(container.querySelector('div')).toBeInTheDocument(); + + // Test unmounting + act(() => { + render(null, { container }); + }); + expect(container.querySelector('div')).not.toBeInTheDocument(); }); it('renders correctly when ref is passed internally', () => { const { container } = render( <CSSMotion motionName="transition" visible> {(props, ref) => <div ref={ref} />} </CSSMotion>, ); - // 检查 DOM 是否渲染出来 expect(container.querySelector('div')).toBeInTheDocument(); + + // Test multiple renders + act(() => { + render( + <CSSMotion motionName="transition" visible> + {(props, ref) => <div ref={ref} data-testid="new" />} + </CSSMotion>, + { container } + ); + }); + expect(container.querySelector('[data-testid="new"]')).toBeInTheDocument(); }); + + it('handles errors gracefully', () => { + const errorSpy = jest.spyOn(console, 'error').mockImplementation(() => {}); + + expect(() => { + render( + <CSSMotion motionName="transition" visible> + {(props, ref) => { + throw new Error('Test error'); + }} + </CSSMotion>, + ); + }).toThrow('Test error'); + + errorSpy.mockRestore(); + });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/CSSMotion.spec.tsx(4 hunks)
🔇 Additional comments (1)
tests/CSSMotion.spec.tsx (1)
12-19: 代码实现清晰且符合最佳实践
ForwardedComponent的实现正确使用了React.forwardRef，并且合理处理了 props 的解构和传递。这很好地支持了移除findDOMNode的目标。
|
New and removed dependencies detected. Learn more about Socket for GitHub ↗︎
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
package.json (1)
50-51: 建议：考虑锁定次要版本目前使用的版本范围
^1.2.0允许自动更新到任何 1.x.x 版本。考虑到这是一个关键的依赖更改，建议锁定次要版本以提高稳定性。- "@rc-component/util": "^1.2.0", + "@rc-component/util": "~1.2.0",
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
package.json(1 hunks)src/CSSMotion.tsx(7 hunks)src/DomWrapper.tsx(0 hunks)src/hooks/useIsomorphicLayoutEffect.ts(1 hunks)src/hooks/useNextFrame.ts(1 hunks)src/hooks/useStatus.ts(1 hunks)src/hooks/useStepQueue.ts(1 hunks)src/util/motion.ts(1 hunks)tests/CSSMotion.spec.tsx(5 hunks)
💤 Files with no reviewable changes (1)
- src/DomWrapper.tsx
✅ Files skipped from review due to trivial changes (5)
- src/hooks/useIsomorphicLayoutEffect.ts
- src/hooks/useStepQueue.ts
- src/hooks/useStatus.ts
- src/hooks/useNextFrame.ts
- src/util/motion.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/CSSMotion.tsx
- tests/CSSMotion.spec.tsx
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/CSSMotion.tsx (1)
228-228: 类型断言的使用建议考虑添加运行时类型检查，以确保返回值始终是有效的 React 元素。
- return motionChildren as React.ReactElement; + if (!React.isValidElement(motionChildren)) { + throw new Error('CSSMotion children must be a valid React element'); + } + return motionChildren;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
package.json(2 hunks)src/CSSMotion.tsx(7 hunks)src/CSSMotionList.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- package.json
🔇 Additional comments (5)
src/CSSMotion.tsx (4)
2-3: 导入新的 DOM 和 ref 工具函数引入
@rc-component/util中的工具函数来替代已弃用的findDOMNode，这是一个很好的改进。
93-93: 更新了 children 回调函数中的 ref 类型将 ref 参数类型从回调函数改为
React.Ref<any>使其与现代 React 的 ref 处理方式保持一致。
141-141: 重构了 DOM 节点获取逻辑
- 使用
getDOM替换了findDOMNode- 通过
useImperativeHandle暴露getDomElement方法这些改动提高了代码的可维护性并遵循了 React 最佳实践。
Also applies to: 159-159
171-175: 统一更新了所有 children 渲染点的 ref 传递所有
children调用点都更新为使用nodeRef，保持了一致性。Also applies to: 180-180, 210-210, 222-222
src/CSSMotionList.tsx (1)
58-58: 更新了列表组件的 ref 类型将 ref 类型更新为
React.Ref<any>，与CSSMotion组件保持一致，这种一致性有助于维护代码质量。
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #59 +/- ##
==========================================
+ Coverage 98.23% 98.44% +0.21%
==========================================
Files 12 11 -1
Lines 396 386 -10
Branches 111 108 -3
==========================================
- Hits 389 380 -9
+ Misses 7 6 -1 ☔ View full report in Codecov by Sentry. |
issue：ant-design/ant-design#52115
Remove findDOMNode from rc-css-motion的问题修复
Summary by CodeRabbit
重构
CSSMotion组件中获取 DOM 元素的方法findDOMNode方法的使用DomWrapper组件，简化了代码结构CSSMotionListProps接口中的childrenprop 类型，以使用标准的 React ref 类型测试
ReactDOM.findDOMNode的依赖这些更改提高了代码的可读性和维护性，同时遵循了现代 React 最佳实践。