Skip to content

refactor: use props check instead of version check#593

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.

Sign up for GitHub

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

Merged
zombieJ merged 4 commits into from
Dec 5, 2024
Merged

refactor: use props check instead of version check #593

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
4c15657
refactor: use props check instead of version check
zombieJ Dec 5, 2024
2061d7f
chore: update peer
zombieJ Dec 5, 2024
2d52941
chore: clean up
zombieJ Dec 5, 2024
ce78edf
chore: update build
zombieJ Dec 5, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 0 additions & 12 deletions now.json
View file
Open in desktop

This file was deleted.

2 changes: 2 additions & 0 deletions package.json
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -67,6 +67,8 @@
"rc-test": "^7.0.14",
"react": "^18.0.0",
"react-dom": "^18.0.0",
"react-19": "npm:react@19.0.0-rc-de68d2f4-20241204",
"react-dom-19": "npm:react-dom@19.0.0-rc-de68d2f4-20241204",
"typescript": "^5.3.2"
},
"peerDependencies": {
Expand Down
26 changes: 10 additions & 16 deletions src/ref.ts
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -81,19 +81,13 @@ export const supportNodeRef = <T = any>(
*/
export const getNodeRef: <T = any>(
node: React.ReactNode,
) => React.Ref<T> | null =
Number(version.split('.')[0]) >= 19
? // >= React 19
node => {
if (isReactElement(node)) {
return (node as any).props.ref;
}
return null;
}
: // < React 19
node => {
if (isReactElement(node)) {
return (node as any).ref;
}
return null;
};
) => React.Ref<T> | null = node => {
if (node && isReactElement(node)) {
const ele = node as any;

// Source from:
// https://github.com/mui/material-ui/blob/master/packages/mui-utils/src/getReactNodeRef/getReactNodeRef.ts
return ele.props.propertyIsEnumerable('ref') ? ele.props.ref : ele.ref;
}
return null;
};
Comment on lines +84 to +93

@coderabbitaicoderabbitaiBotDec 5, 2024

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

建议优化 ref 获取逻辑的实现

当前实现存在以下问题：

  1. 直接调用目标对象的 propertyIsEnumerable 方法可能不安全
  2. 新增的返回 null 分支缺少测试覆盖

建议进行如下优化：

export const getNodeRef: <T = any>(
node: React.ReactNode,
) => React.Ref<T> | null = node => {
if (node && isReactElement(node)) {
const ele = node as any;
- return ele.props.propertyIsEnumerable('ref') ? ele.props.ref : ele.ref;+ return Object.prototype.propertyIsEnumerable.call(ele.props, 'ref') + ? ele.props.ref + : ele.ref;
}
return null;
};
📝 Committable suggestion

‼️IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
)=>React.Ref<T>|null=node=>{
if(node&&isReactElement(node)){
constele=nodeasany;
// Source from:
// https://github.com/mui/material-ui/blob/master/packages/mui-utils/src/getReactNodeRef/getReactNodeRef.ts
returnele.props.propertyIsEnumerable('ref') ? ele.props.ref : ele.ref;
}
returnnull;
};
)=>React.Ref<T>|null=node=>{
if(node&&isReactElement(node)){
constele=nodeasany;
// Source from:
// https://github.com/mui/material-ui/blob/master/packages/mui-utils/src/getReactNodeRef/getReactNodeRef.ts
returnObject.prototype.propertyIsEnumerable.call(ele.props,'ref')
? ele.props.ref
: ele.ref;
}
returnnull;
};
🧰 Tools
🪛 Biome (1.9.4)

[error] 90-90: Do not access Object.prototype method 'propertyIsEnumerable' from target object.

(lint/suspicious/noPrototypeBuiltins)

🪛 GitHub Check: codecov/patch

[warning] 92-92: src/ref.ts#L92
Added line #L92 was not covered by tests

30 changes: 30 additions & 0 deletions tests/ref-19.test.tsx
View file
Open in desktop
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,30 @@
/* eslint-disable no-eval */
import React from 'react';
import { getNodeRef } from '../src/ref';

jest.mock('react', () => {
const react19 = jest.requireActual('react-19');
return react19;
});

jest.mock('react-dom', () => {
const reactDom19 = jest.requireActual('react-dom-19');
return reactDom19;
});

describe('ref: React 19', () => {
const errSpy = jest.spyOn(console, 'error');

beforeEach(() => {
errSpy.mockReset();
});

it('getNodeRef', () => {
const ref = React.createRef<HTMLDivElement>();
const node = <div ref={ref} />;

expect(getNodeRef(node)).toBe(ref);

expect(errSpy).not.toHaveBeenCalled();
});
Comment on lines +22 to +29

@coderabbitaicoderabbitaiBotDec 5, 2024

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

建议增加更多测试用例

当前测试只覆盖了基本场景，建议添加以下测试用例：

  1. 传入 null/undefined 节点的情况
  2. 传入非 React 元素的情况
  3. 不同类型的 ref（函数式、对象式）

示例测试用例：

it('should handle null/undefined',()=>{expect(getNodeRef(null)).toBe(null);expect(getNodeRef(undefined)).toBe(null);});it('should handle non-React elements',()=>{expect(getNodeRef('string')).toBe(null);expect(getNodeRef(123)).toBe(null);});it('should work with function ref',()=>{constfnRef=jest.fn();constnode=<divref={fnRef}/>;expect(getNodeRef(node)).toBe(fnRef);});

});
8 changes: 8 additions & 0 deletions tests/ref.test.tsx
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -12,6 +12,12 @@ import {
} from '../src/ref';

describe('ref', () => {
const errSpy = jest.spyOn(console, 'error');

beforeEach(() => {
errSpy.mockReset();
});

describe('composeRef', () => {
it('basic', () => {
const refFunc1 = jest.fn();
Expand DownExpand Up@@ -207,5 +213,7 @@ describe('ref', () => {
const node = <div ref={ref} />;

expect(getNodeRef(node)).toBe(ref);

expect(errSpy).not.toHaveBeenCalled();
});
});