-
Notifications
You must be signed in to change notification settings - Fork 45
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
解决findDOMNode 过期警告问题 #215
base: master
Are you sure you want to change the base?
解决findDOMNode 过期警告问题 #215
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
概述遍历此次拉取请求引入了一个新的 React 组件 变更
序列图sequenceDiagram
participant React App
participant HTMLComment
participant Ref
participant SingleObserver
React App->>HTMLComment: 创建注释节点
HTMLComment-->>React App: 返回注释节点
React App->>Ref: 传入子组件和引用
Ref->>Ref: 使用 MutationObserver 监听 DOM 变化
Ref-->>React App: 管理和更新引用
React App->>SingleObserver: 渲染组件
SingleObserver->>Ref: 使用新的引用处理机制
SingleObserver-->>React App: 渲染完成
可能相关的 PR
建议的审阅者
诗歌
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
💤 Files with no reviewable changes (1)
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
🧹 Nitpick comments (5)
src/SingleObserver/HTMLComment.tsx (2)
3-5
: 接口设计建议:为 data 提供更强的类型约束。
当前仅声明了可选字符串类型,若后续需要不同的注释内容格式或增加额外数据属性,可以考虑使用更精确的类型或增加注释说明。
25-33
: 注释节点创建逻辑可拓展性不足。
目前仅通过固定的正则与 TagSymbol 匹配,如果后续需要为注释节点添加更多元信息(例如动态配置属性),需要修改此处逻辑。可考虑将相关处理封装在一个独立函数,以便后续拓展。src/SingleObserver/Ref.tsx (1)
75-83
: 在 JSX 中可以考虑添加容错处理。
如果 ref 已经在其他地方被使用,在渲染时出错或者发生异常,会导致无法正确调用 assignRef。建议在此处为 ref 的获取或赋值增加一些错误处理或兜底逻辑,以免出现空引用问题。src/SingleObserver/index.tsx (2)
16-16
: 将 wrapperRef 的类型由 Element | Text | null 替换为更具扩展性的泛型类型。
如果未来需要监听其他类型的dom(比如 SVGElement 等),可以考虑使用范型或更宽松的类型声明,以增强可维护性。
111-119
: 条件渲染判断可以合并或简化。
在 canRef 分支与 fallback 分支中,渲染逻辑略显重复。若后续还需要根据更多布尔条件进行判断,可考虑将该逻辑抽取到一个函数中,让代码更具可读性与可扩展性。
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/SingleObserver/HTMLComment.tsx
(1 hunks)src/SingleObserver/Ref.tsx
(1 hunks)src/SingleObserver/index.tsx
(4 hunks)
🔇 Additional comments (4)
src/SingleObserver/HTMLComment.tsx (1)
50-54
: React.memo 配合 forwardRef 使用正常,但需谨慎确定是否真的需要 memo。
由于该组件的预期行为是生成注释节点,同时只依赖 data 属性,memo 确实可对性能进行微优化。但若后续存在更多动态属性或内部逻辑变化,可能需要重新评估使用场景。
src/SingleObserver/Ref.tsx (2)
6-15
: updateRef 函数确保兼容多种 ref 类型,逻辑清晰。
此处正确区分了函数式 ref 和对象式 ref,能很好地在多种场景中正常使用。实现上无需额外改动,符合最佳实践。
25-35
: 对于 resolveContent 函数的可读性和安全性可再斟酌。
- 通过 nextSibling 获取内容节点时,假设了注释节点相邻的结构一定固定,这可能在 DOM 结构改变时有潜在风险。
- 若需要处理更多情况(例如注释批量插入、文本节点被替换等),可以考虑在这里提供更多防御式校验。
src/SingleObserver/index.tsx (1)
45-45
: ensure getDom 不依赖 findDOMNode 的最终结果时的使用场景。
虽然此处依然保留了 findDOMNode 的调用,但若 elementRef.current 能直接获取到正确的元素,理论上可以省略 findDOMNode。后续如完全去除 findDOMNode,需要确认依赖场景已经迁移完成。
✅ Verification successful
Let me gather more context about the usage of findDOMNode in these files.
Based on the code analysis, I can now provide the final response:
需要保留 findDOMNode 调用以确保兼容性
当前 getDom()
函数中的 findDOMNode 调用是必要的,原因如下:
- 需要处理 Angular 的 nativeElement 场景
- 作为降级方案确保在 elementRef 无法直接获取 DOM 元素时的可用性
- DomWrapper 组件专门用于处理这种降级场景
建议:
- 保持现有实现,因为这是一个合理的兼容性设计
- 后续如需完全移除 findDOMNode,需要先确保:
- Angular 集成场景有替代方案
- 所有使用场景都能通过 ref 直接获取到 DOM 元素
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# 搜索全代码库中 findDOMNode 其他引用场景,确保已逐步移除
rg "findDOMNode"
Length of output: 393
Script:
#!/bin/bash
# Check the implementation in DomWrapper.tsx
cat src/SingleObserver/DomWrapper.tsx
# Check the full implementation in index.tsx
cat src/SingleObserver/index.tsx
Length of output: 4676
src/SingleObserver/HTMLComment.tsx
Outdated
//react 内部是用这个创建节点的 由于react本身不支持创建注释节点 这里hack一下 | ||
document.createElement = function ( | ||
tagName: string, | ||
options?: ElementCreationOptions | ||
) { | ||
if ( | ||
tagName === "noscript" && | ||
options?.is && | ||
options.is.startsWith(TagSymbol) | ||
) { | ||
const regex = new RegExp(`^${TagSymbol}(.*)$`); | ||
const match = options?.is.match(regex); | ||
if (match) { | ||
const data = match[1].trim?.(); | ||
const comment = document.createComment(data) as unknown as ElementLike; | ||
comment.setAttribute = () => true; | ||
comment.style = {}; | ||
return comment as unknown as HTMLElement; | ||
} | ||
} | ||
return createElement.call(this, tagName, options); | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
对 document.createElement 的黑客式重写需谨慎。
重写全局创建元素的方法,会影响整份代码中其他组件或第三方库对 document.createElement 的使用,可能在将来带来意外的兼容性或维护挑战。建议在局部范围内封装或使用更小的自定义逻辑,而非覆盖全局。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 最好不要污染原生方法。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@afc163 感谢提醒!在生产环境中全局覆盖 document.createElement
的确存在潜在风险,尤其是在大型项目或使用第三方库时,都可能遇到不可预期的问题。为避免污染原生方法,一个更稳妥的做法是只在组件内处理想要生成注释节点的逻辑,比如通过自定义函数或自定义渲染逻辑直接调用 document.createComment
,而不是覆盖全局的 createElement
方法。这样既能保留在组件局部创建注释节点的需求,也能使项目保持纯净,减少维护成本和不可预期的风险。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@coderabbitai 这个文件如果不污染 document.createElement,又要实现同样的效果,可以怎么写?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@afc163 可以考虑在组件内部直接生成并替换一个占位节点,而不是全局覆盖 document.createElement
。下面是一个示例思路:
• 在渲染时先返回一个普通的元素(例如一个空的 ),提供给 React 以便正常挂载。
• 在组件挂载后(useEffect 内),创建注释节点并替换掉之前渲染的占位元素。
• 如果需要更新注释内容,可在依赖更新后修改注释节点的 data 字段。
示例实现如下:
import React from "react";
export interface CommentProps {
data?: string;
}
function HTMLComment({ data }: CommentProps) {
const placeholderRef = React.useRef<HTMLSpanElement | null>(null);
const commentNodeRef = React.useRef<Comment | null>(null);
React.useEffect(() => {
// 如果尚未创建过注释节点,则创建并替换掉占位节点
if (!commentNodeRef.current) {
const node = document.createComment(data || "");
commentNodeRef.current = node;
if (placeholderRef.current?.parentNode) {
placeholderRef.current.parentNode.replaceChild(node, placeholderRef.current);
}
} else {
// 已有注释节点时,更新其内容
commentNodeRef.current.data = data || "";
}
}, [data]);
// 返回一个空的占位元素,便于挂载
return <span ref={placeholderRef} style={{ display: "none" }} />;
}
export default React.memo(HTMLComment);
如上做法不会污染全局的 document.createElement
,也能在最终的 DOM 中实现注释节点。原理就是先用一个占位 DOM,让 React 正常挂载。然后在挂载完成后再通过原生 DOM API 创建注释节点并将其替换。这样你就能在组件内局部使用注释节点,而无需对全局进行 Hack。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
这种写法我试过 react17和react18 会有差异 react17 需要去重写placeholderRef.ccurent 节点的appendChild,replaceChild 方法 react 17 才不会报错 实现方法有点诡异
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
// 创建 `MutationObserver`,监听 DOM 子节点变化 | ||
const ob = new MutationObserver((mutations) => { | ||
if (parent) { | ||
mutations.forEach((m) => { | ||
m.removedNodes.forEach((node) => { | ||
if (node === contentsRef.current) { | ||
updateRef(null, ref); // 当 DOM 变化时更新 ref | ||
contentsRef.current = null; | ||
} | ||
}); | ||
m.addedNodes.forEach(() => { | ||
assignRef(); | ||
}); | ||
}); | ||
} | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
MutationObserver 回调中未对 MutationRecord 的 target 做更多限制。
目前仅基于 removedNodes 和 addedNodes 判断。如果后续要处理属性突变(attributes: true)或结构更复杂的变动,需要进一步细化判断逻辑。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
这里只关系节点的移除和新增 属性变化不关系
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@robertpanvip,明白了,感谢你的说明。仅基于节点的增删场景即可满足当前需求,如果以后需要处理属性或其它更复杂的变动逻辑,我们可以再进行细化。
✏️ Learnings added
Learnt from: robertpanvip
PR: react-component/resize-observer#215
File: src/SingleObserver/Ref.tsx:47-62
Timestamp: 2024-12-19T07:27:01.427Z
Learning: 在 Ref.tsx 中的 MutationObserver 回调中,仅关心节点的增删(removedNodes 和 addedNodes),不处理属性变化。
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/SingleObserver/HTMLComment.tsx
(1 hunks)
🔇 Additional comments (3)
src/SingleObserver/HTMLComment.tsx (3)
29-33
: 谨慎重写全局 document.createElement
方法。
在全局范围内重写此方法可能会对其他依赖该方法的第三方库或组件产生意料之外的影响,可能导致后期维护和兼容性问题。若可能,建议改为局部拦截或通过自定义渲染函数等方式处理注释节点,以减少对系统全局行为的影响。
14-20
: 使用 canUseDom
判断时需要考虑 SSR 场景。
如果在服务端渲染时使用该组件,需要确保不会潜在调用到 document.createElement
,以避免在非浏览器环境抛出异常。建议在 SSR 环境中跳过相关逻辑或使用其他安全检测方式。
73-77
: 使用 React.memo 与 forwardRef 的结合实现优化渲染。
在此组件中使用 React.memo
并结合 forwardRef
的做法是一个不错的性能优化点,有助于减少不必要的重新渲染。请确保外部组件或调用方不会频繁改变 data
值,否则依旧会触发重渲染。
const comment = document.createComment(data) as unknown as ElementLike; | ||
comment.setAttribute = () => true; | ||
comment.style = {}; | ||
reset(); | ||
return comment as unknown as HTMLElement; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
慎用 reset()
的时机与顺序。
在创建注释节点后第一时间恢复 document.createElement
可能导致其他部分还未执行的逻辑出现无法创建注释节点的情况。请确认在创建注释节点的完整生命周期内,是否有必要维持重写状态以兼容后续操作或复合调用场景。
mark |
解决findDOMNode 过期警告问题
issues: #208
issues: ant-design/ant-design#48709
issues: react-component/trigger#187
Summary by CodeRabbit
Summary by CodeRabbit
新功能
HTMLComment
组件,用于在 React 应用中创建注释节点。Ref
组件,支持对 DOM 元素的引用管理。改进
SingleObserver
组件的结构和引用处理逻辑,简化了 DOM 节点的获取方式。DomWrapper
组件,优化了组件的整体结构。