feat: Remove findDOMNode from rc-css-motion #59
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
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的目标。
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
issue：ant-design/ant-design#52115
Remove findDOMNode from rc-css-motion的问题修复
Summary by CodeRabbit
重构
CSSMotion组件中获取 DOM 元素的方法findDOMNode方法的使用测试
ReactDOM.findDOMNode的依赖这些更改提高了代码的可读性和维护性，同时遵循了现代 React 最佳实践。