Skip to content

Avoid rc package deep imports #164

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

Open
QDyanbing wants to merge 1 commit into
base: master
Choose a base branch
from
+81 −67
Open

Avoid rc package deep imports #164

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
14 changes: 5 additions & 9 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,22 +43,19 @@
]
},
"dependencies": {
"@rc-component/util": "^1.3.0",
"@rc-component/util": "^1.11.1",
"clsx": "^2.1.1"
},
"devDependencies": {
"@rc-component/father-plugin": "^2.0.0",
"@rc-component/father-plugin": "^2.2.0",
"@rc-component/np": "^1.0.3",
"@testing-library/react": "^16.0.1",
"@types/jest": "^29.4.0",
"@types/node": "^24.5.2",
"@types/react": "^19.1.14",
"@types/react-dom": "^19.1.9",
"@umijs/fabric": "^3.0.0",
"cheerio": "1.0.0-rc.12",
"dumi": "^2.0.0",
"enzyme": "^3.0.0",
"enzyme-adapter-react-16": "^1.0.1",
"enzyme-to-json": "^3.0.0",
"eslint": "^8.55.0",
"eslint-plugin-jest": "^27.6.0",
"eslint-plugin-unicorn": "^49.0.0",
Expand All @@ -68,9 +65,8 @@
"less": "^4.1.3",
"lint-staged": "^15.1.0",
"prettier": "^3.1.0",
"react": "^16.0.0",
"react-dom": "^16.0.0",
"react-test-renderer": "^16.0.0",
"react": "^18.0.0",
"react-dom": "^18.0.0",
"umi-test": "^1.9.7"
},
"peerDependencies": {
Expand Down
9 changes: 5 additions & 4 deletions src/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,16 +1,17 @@
import * as React from 'react';
import { clsx } from 'clsx';
import useControlledState from '@rc-component/util/lib/hooks/useControlledState';
import KeyCode from '@rc-component/util/lib/KeyCode';
import { KeyCode, useControlledState } from '@rc-component/util';

export type SwitchChangeEventHandler = (
checked: boolean,
event: React.MouseEvent<HTMLButtonElement> | React.KeyboardEvent<HTMLButtonElement>,
) => void;
export type SwitchClickEventHandler = SwitchChangeEventHandler;

interface SwitchProps
extends Omit<React.HTMLAttributes<HTMLButtonElement>, 'onChange' | 'onClick'> {
interface SwitchProps extends Omit<
React.HTMLAttributes<HTMLButtonElement>,
'onChange' | 'onClick'
> {
className?: string;
prefixCls?: string;
disabled?: boolean;
Expand Down
125 changes: 71 additions & 54 deletions tests/index.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,15 @@
import React from 'react';
import KeyCode from '@rc-component/util/lib/KeyCode';
import { mount } from 'enzyme';
import { KeyCode } from '@rc-component/util';
import { fireEvent, render } from '@testing-library/react';
import Switch from '..';

describe('rc-switch', () => {
function keyDownWithWhich(target, which) {
fireEvent.keyDown(target, { keyCode: which });
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot May 21, 2026

Choose a reason for hiding this comment

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

medium

建议在 fireEvent.keyDown 中显式传入 which 属性。虽然 keyCode 已被弃用，但由于组件内部逻辑（如 src/index.tsx 第 74 行）仍在使用 e.which，在测试中模拟该属性可以确保与组件逻辑的一致性并提高测试的健壮性。

Suggested change
fireEvent.keyDown(target, { keyCode: which });
fireEvent.keyDown(target, { keyCode: which, which });

}

function createSwitch(props = {}) {
return mount(
return render(
<Switch
checkedChildren={<span className="checked" />}
unCheckedChildren={<span className="unchecked" />}
Expand All @@ -15,127 +19,140 @@ describe('rc-switch', () => {
}

it('works', () => {
const wrapper = createSwitch();
expect(wrapper.exists('.unchecked')).toBeTruthy();
wrapper.simulate('click');
expect(wrapper.exists('.checked')).toBeTruthy();
const { getByRole } = createSwitch();
const switchNode = getByRole('switch');

expect(switchNode.getAttribute('aria-checked')).toBe('false');
fireEvent.click(switchNode);
expect(switchNode.getAttribute('aria-checked')).toBe('true');
});

it('should be checked upon right key and unchecked on left key', () => {
const wrapper = createSwitch();
expect(wrapper.exists('.unchecked')).toBeTruthy();
wrapper.simulate('keydown', { which: KeyCode.RIGHT });
expect(wrapper.exists('.checked')).toBeTruthy();
wrapper.simulate('keydown', { which: KeyCode.LEFT });
expect(wrapper.exists('.unchecked')).toBeTruthy();
const { getByRole } = createSwitch();
const switchNode = getByRole('switch');

expect(switchNode.getAttribute('aria-checked')).toBe('false');
keyDownWithWhich(switchNode, KeyCode.RIGHT);
expect(switchNode.getAttribute('aria-checked')).toBe('true');
keyDownWithWhich(switchNode, KeyCode.LEFT);
expect(switchNode.getAttribute('aria-checked')).toBe('false');
});

it('should change from an initial checked state of true to false on click', () => {
const onChange = jest.fn();
const wrapper = createSwitch({ defaultChecked: true, onChange });
expect(wrapper.exists('.checked')).toBeTruthy();
wrapper.simulate('click');
expect(wrapper.exists('.unchecked')).toBeTruthy();
const { getByRole } = createSwitch({ defaultChecked: true, onChange });
const switchNode = getByRole('switch');

expect(switchNode.getAttribute('aria-checked')).toBe('true');
fireEvent.click(switchNode);
expect(switchNode.getAttribute('aria-checked')).toBe('false');
expect(onChange.mock.calls.length).toBe(1);
});

it('should support onClick', () => {
const onClick = jest.fn();
const wrapper = createSwitch({ onClick });
wrapper.simulate('click');
const { getByRole } = createSwitch({ onClick });
const switchNode = getByRole('switch');

fireEvent.click(switchNode);
expect(onClick).toHaveBeenCalledWith(true, expect.objectContaining({ type: 'click' }));
expect(onClick.mock.calls.length).toBe(1);
wrapper.simulate('click');
fireEvent.click(switchNode);
expect(onClick).toHaveBeenCalledWith(false, expect.objectContaining({ type: 'click' }));
expect(onClick.mock.calls.length).toBe(2);
});

it('should not toggle when clicked in a disabled state', () => {
const onChange = jest.fn();
const wrapper = createSwitch({ disabled: true, checked: true, onChange });
expect(wrapper.exists('.checked')).toBeTruthy();
wrapper.simulate('click');
expect(wrapper.exists('.checked')).toBeTruthy();
const { getByRole } = createSwitch({ disabled: true, checked: true, onChange });
const switchNode = getByRole('switch');

expect(switchNode.getAttribute('aria-checked')).toBe('true');
fireEvent.click(switchNode);
expect(switchNode.getAttribute('aria-checked')).toBe('true');
expect(onChange.mock.calls.length).toBe(0);
});

it('should support loading icon node', () => {
const wrapper = mount(<Switch loadingIcon={<span className="extra">loading</span>} />);
expect(wrapper.find('.extra').text()).toBe('loading');
const { container } = render(<Switch loadingIcon={<span className="extra">loading</span>} />);
expect(container.querySelector('.extra').textContent).toBe('loading');
Comment on lines +77 to +78
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot May 21, 2026

Choose a reason for hiding this comment

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

medium

建议使用 React Testing Library 推荐的查询方式（如 getByText）来替代 container.querySelector。这不仅更符合 RTL 的最佳实践（从用户视角出发），而且在元素未找到时能提供更清晰的错误信息。

Suggested change
const { container } = render(<Switch loadingIcon={<span className="extra">loading</span>} />);
expect(container.querySelector('.extra').textContent).toBe('loading');
const { getByText } = render(<Switch loadingIcon={<span className="extra">loading</span>} />);
expect(getByText('loading').classList.contains('extra')).toBe(true);

});

it('focus()', () => {
const container = document.createElement('div');
document.body.appendChild(container);
const handleFocus = jest.fn();
const ref = React.createRef();
mount(<Switch ref={ref} onFocus={handleFocus} />, {
attachTo: container,
});
render(<Switch ref={ref} onFocus={handleFocus} />);

ref.current.focus();
expect(document.activeElement).toBe(ref.current);
fireEvent.focus(ref.current);
expect(handleFocus).toHaveBeenCalled();
});

it('blur()', () => {
const container = document.createElement('div');
document.body.appendChild(container);
const handleBlur = jest.fn();
const ref = React.createRef();
mount(<Switch ref={ref} onBlur={handleBlur} />, {
attachTo: container,
});
render(<Switch ref={ref} onBlur={handleBlur} />);

ref.current.focus();
expect(document.activeElement).toBe(ref.current);
ref.current.blur();
expect(document.activeElement).not.toBe(ref.current);
fireEvent.blur(ref.current);
expect(handleBlur).toHaveBeenCalled();
});

describe('autoFocus', () => {
it('basic', () => {
const container = document.createElement('div');
document.body.appendChild(container);
const handleFocus = jest.fn();
mount(<Switch autoFocus onFocus={handleFocus} />, { attachTo: container });
const { getByRole } = render(<Switch autoFocus onFocus={handleFocus} />);
const switchNode = getByRole('switch');

expect(document.activeElement).toBe(switchNode);
fireEvent.focus(switchNode);
expect(handleFocus).toHaveBeenCalled();
});

it('not work when disabled', () => {
const container = document.createElement('div');
document.body.appendChild(container);
const handleFocus = jest.fn();
mount(<Switch autoFocus disabled onFocus={handleFocus} />, { attachTo: container });
render(<Switch autoFocus disabled onFocus={handleFocus} />);
expect(handleFocus).not.toHaveBeenCalled();
});
});

it('disabled', () => {
const wrapper = createSwitch({ disabled: true });
expect(wrapper.exists('.unchecked')).toBeTruthy();
wrapper.simulate('keydown', { keyCode: 39 });
expect(wrapper.exists('.unchecked')).toBeTruthy();
const { getByRole } = createSwitch({ disabled: true });
const switchNode = getByRole('switch');

expect(switchNode.getAttribute('aria-checked')).toBe('false');
keyDownWithWhich(switchNode, KeyCode.RIGHT);
expect(switchNode.getAttribute('aria-checked')).toBe('false');
});

it('onMouseUp', () => {
const onMouseUp = jest.fn();
const wrapper = createSwitch({ onMouseUp });
wrapper.simulate('mouseup');
const { getByRole } = createSwitch({ onMouseUp });

fireEvent.mouseUp(getByRole('switch'));
expect(onMouseUp).toHaveBeenCalled();
});

it('disabled should click not to change', () => {
const onChange = jest.fn();
const onClick = jest.fn();
const wrapper = createSwitch({ disabled: true, onChange, onClick, checked: true });
const { getByRole } = createSwitch({ disabled: true, onChange, onClick, checked: true });

wrapper.simulate('click');
fireEvent.click(getByRole('switch'));
expect(onChange).not.toHaveBeenCalled();
});
Comment on lines 140 to 147
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot May 21, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

考虑验证 onClick 回调也未被调用。

测试中定义了 onClick 回调（第 142 行），但只断言了 onChange 未被调用。为了更完整的测试覆盖，建议也验证禁用状态下 onClick 的行为。

💡 建议添加 onClick 断言 
 fireEvent.click(getByRole('switch'));
 expect(onChange).not.toHaveBeenCalled();
+expect(onClick).not.toHaveBeenCalled();
📝 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
it('disabled should click not to change', () => {
const onChange = jest.fn();
const onClick = jest.fn();
const wrapper = createSwitch({ disabled: true, onChange, onClick, checked: true });
const { getByRole } = createSwitch({ disabled: true, onChange, onClick, checked: true });
wrapper.simulate('click');
fireEvent.click(getByRole('switch'));
expect(onChange).not.toHaveBeenCalled();
});
it('disabled should click not to change', () => {
const onChange = jest.fn();
const onClick = jest.fn();
const { getByRole } = createSwitch({ disabled: true, onChange, onClick, checked: true });
fireEvent.click(getByRole('switch'));
expect(onChange).not.toHaveBeenCalled();
expect(onClick).not.toHaveBeenCalled();
});
🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/index.spec.js` around lines 140 - 147, The test "disabled should click
not to change" defines an onClick mock via onClick and fires a click on the
switch but only asserts onChange wasn't called; update the test that uses
createSwitch({ disabled: true, onChange, onClick, checked: true }) to also
assert that the onClick mock was not called after
fireEvent.click(getByRole('switch')), i.e., add
expect(onClick).not.toHaveBeenCalled() to the test to verify disabled state
prevents click handlers from running.

it('support classnames and styles', () => {
const wrapper = createSwitch({
const { container } = createSwitch({
classNames: { content: 'custom-content' },
styles: { content: { background: 'red' } },
});
const contentElement = wrapper.find('.custom-content').at(0);
expect(contentElement.exists()).toBe(true);
expect(contentElement.prop('style')).toEqual({ background: 'red' });
const contentElement = container.querySelector('.custom-content');

expect(contentElement).toBeTruthy();
expect(contentElement.style.background).toBe('red');
});
});