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
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 1 commit
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
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff 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 number Diff line number Diff line change
Expand Up @@ -81,19 +81,13 @@
*/
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;

Check warning on line 92 in src/ref.ts

View check run for this annotation

Codecov / codecov/patch

src/ref.ts#L92 

Added line #L92 was not covered by tests
};
Comment on lines +84 to +93
Copy link

@coderabbitai coderabbitai bot Dec 5, 2024

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)) {
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;
};
) => 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 Object.prototype.propertyIsEnumerable.call(ele.props, 'ref')
? ele.props.ref
: ele.ref;
}
return null;
};
🧰 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 number Diff line number Diff 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
Copy link

@coderabbitai coderabbitai bot Dec 5, 2024

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', () => { const fnRef = jest.fn(); const node = <div ref={fnRef} />; expect(getNodeRef(node)).toBe(fnRef); });
});
8 changes: 8 additions & 0 deletions tests/ref.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff 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 Down Expand Up @@ -207,5 +213,7 @@ describe('ref', () => {
const node = <div ref={ref} />;

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

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