Remove findDOMNode from rc-mutate-observer #14
Conversation
概述代码走查本次更改主要涉及 变更
诗歌
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 (
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
src/MutateObserver.tsx (2)
31-37: 优化目标元素的设置逻辑新的实现成功移除了
findDOMNode，代码更加清晰。不过，我们可以通过缓存canRef的计算结果来进一步优化性能。建议如下优化：
- const canRef = React.isValidElement(children) && supportRef(children); + const canRef = React.useMemo( + () => React.isValidElement(children) && supportRef(children), + [children] + );
Line range hint
41-44: 改进错误消息的可读性当前的错误消息可以更加具体，以帮助开发者更快地定位问题。
建议如下修改：
- console.error('MutationObserver need children props'); + console.error('MutationObserver requires children prop to be provided. Received: null');tests/index.test.tsx (2)
39-41: 建议改进测试清理逻辑虽然在测试结束时恢复了
console.error，但建议使用afterEach来确保在测试失败时也能正确清理。+ beforeEach(() => { + jest.spyOn(console, 'error').mockImplementation(() => {}); + }); + + afterEach(() => { + jest.restoreAllMocks(); + }); + it('MutateObserver should work without errors in React.StrictMode', () => { const fn = jest.fn(); const buttonRef = React.createRef<HTMLButtonElement>(); - - // Mock console.error to ensure no warnings are logged - const warnSpy = jest.spyOn(console, 'error').mockImplementation(() => {});
65-70: 建议增加更具体的断言当前的断言验证了没有警告，但可以添加更多断言来验证组件的具体行为。
建议添加以下断言：
// 验证按钮类名的变化 expect(container.querySelector('button')).toHaveClass('bbb'); // 验证回调函数的参数 if ('MutationObserver' in window) { expect(fn).toHaveBeenCalledWith( expect.arrayContaining([ expect.objectContaining({ type: 'attributes', attributeName: 'class' }) ]) ); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/MutateObserver.tsx(2 hunks)tests/index.test.tsx(4 hunks)
🔇 Additional comments (2)
src/MutateObserver.tsx (1)
1-7: 导入语句的重组看起来不错！导入语句的重组提高了代码的可读性，同时成功移除了
findDOMNode的依赖。tests/index.test.tsx (1)
Line range hint
23-30: 测试用例的改进很棒！添加的注释和浏览器兼容性检查提高了测试的可读性和可靠性。
|
@Roxannej 冲突了 |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #14 +/- ##
==========================================
- Coverage 78.94% 78.78% -0.16%
==========================================
Files 3 2 -1
Lines 38 33 -5
Branches 13 11 -2
==========================================
- Hits 30 26 -4
Misses 7 7
+ Partials 1 0 -1 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/MutateObserver.tsx (2)
18-24: 建议增强类型安全性当前实现已经很好地支持了 HTML 和 SVG 元素，但我们可以进一步改进类型定义。
建议添加以下类型定义来增强代码的可维护性：
+type SupportedElement = HTMLElement | SVGElement; -const elementRef = React.useRef<HTMLElement | SVGElement>(null); +const elementRef = React.useRef<SupportedElement>(null); -const [target, setTarget] = React.useState<HTMLElement | SVGElement>(null); +const [target, setTarget] = React.useState<SupportedElement>(null);
30-34: 建议添加错误处理当前的实现可能在某些边缘情况下不够健壮。
建议添加错误处理和日志记录：
if (canRef && elementRef.current) { - setTarget(getDOM(elementRef.current)); + try { + const domNode = getDOM(elementRef.current); + if (!domNode && process.env.NODE_ENV !== 'production') { + console.warn('无法获取目标 DOM 节点'); + } + setTarget(domNode); + } catch (error) { + if (process.env.NODE_ENV !== 'production') { + console.error('获取 DOM 节点时发生错误:', error); + } + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/MutateObserver.tsx(3 hunks)
🧰 Additional context used
🪛 GitHub Check: codecov/patch
src/MutateObserver.tsx
[warning] 46-46: src/MutateObserver.tsx#L46
Added line #L46 was not covered by tests
🔇 Additional comments (2)
src/MutateObserver.tsx (2)
1-9: 导入更改看起来不错！导入更改符合 React 最佳实践，通过使用
getDOM替换已弃用的findDOMNode。
44-46: 需要增加测试覆盖率静态分析显示第 46 行缺少测试覆盖。
建议添加以下测试场景：
- 当
canRef为false时的渲染情况- 确保
children在不支持 ref 时保持不变需要我帮您生成相应的测试用例吗？
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 46-46: src/MutateObserver.tsx#L46
Added line #L46 was not covered by tests
Summary by CodeRabbit
新功能
useMutateObserver函数现在支持处理 SVG 元素。测试改进
MutateObserver组件的测试用例代码重构
findDOMNode的依赖useLayoutEffect钩子的实现DomWrapper组件