Skip to content

Commit 7359627

Browse files
shezhangzhangshezhangzhang
authored
chore: Warning user that if option has null value (#778)
* chore: add prettier latest version to devDependencies because the current version can't deal with ts import type syntax * feat: add option null value warning * feat: add null value in options warning in Select * Update Select.tsx * fix: only throw warning in development environment * fix: fix select.test type problems and add null in value warning test case * refactor: optimized null option warning * fix: support OptGroup options * demo: remove useless code * demo: drop controlled example updates * refactor: using recursive to replace for loop * fix: fix null value in fieldNames * refactor: refactor null warning recursive
1 parent 70500ca commit 7359627

File tree

4 files changed

+155
-52
lines changed

4 files changed

+155
-52
lines changed

‎package.json‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -66,6 +66,7 @@
6666
"father": "^2.13.2",
6767
"jsonp": "^0.2.1",
6868
"np": "^7.5.0",
69+
"prettier": "^2.7.1",
6970
"rc-dialog": "^8.1.1",
7071
"typescript": "^4.2.3"
7172
}

‎src/Select.tsx‎

Lines changed: 19 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -29,24 +29,29 @@
2929
* - `combobox` mode not support `optionLabelProp`
3030
*/
3131

32-
import * as React from 'react';
33-
import warning from 'rc-util/lib/warning';
3432
import useMergedState from 'rc-util/lib/hooks/useMergedState';
33+
import warning from 'rc-util/lib/warning';
34+
import * as React from 'react';
35+
import type {
36+
BaseSelectProps,
37+
BaseSelectPropsWithoutPrivate,
38+
BaseSelectRef,
39+
DisplayValueType,
40+
RenderNode,
41+
} from './BaseSelect';
3542
import BaseSelect, { isMultiple } from './BaseSelect';
36-
import type { DisplayValueType, RenderNode } from './BaseSelect';
37-
import OptionList from './OptionList';
38-
import Option from './Option';
39-
import OptGroup from './OptGroup';
40-
import type { BaseSelectRef, BaseSelectPropsWithoutPrivate, BaseSelectProps } from './BaseSelect';
41-
import useOptions from './hooks/useOptions';
42-
import SelectContext from './SelectContext';
43+
import useCache from './hooks/useCache';
44+
import useFilterOptions from './hooks/useFilterOptions';
4345
import useId from './hooks/useId';
46+
import useOptions from './hooks/useOptions';
4447
import useRefFunc from './hooks/useRefFunc';
45-
import { fillFieldNames, flattenOptions, injectPropsWithOption } from './utils/valueUtil';
46-
import warningProps from './utils/warningPropsUtil';
48+
import OptGroup from './OptGroup';
49+
import Option from './Option';
50+
import OptionList from './OptionList';
51+
import SelectContext from './SelectContext';
4752
import { toArray } from './utils/commonUtil';
48-
import useFilterOptions from './hooks/useFilterOptions';
49-
import useCache from './hooks/useCache';
53+
import { fillFieldNames, flattenOptions, injectPropsWithOption } from './utils/valueUtil';
54+
import warningProps, { warningNullOptions } from './utils/warningPropsUtil';
5055

5156
const OMIT_DOM_PROPS = ['inputValue'];
5257

@@ -595,6 +600,7 @@ const Select = React.forwardRef(
595600
// ========================== Warning ===========================
596601
if (process.env.NODE_ENV !== 'production') {
597602
warningProps(props);
603+
warningNullOptions(mergedOptions, mergedFieldNames);
598604
}
599605

600606
// ==============================================================

‎src/utils/warningPropsUtil.ts‎

Lines changed: 39 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,17 @@
1-
import * as React from 'react';
2-
import warning, { noteOnce } from 'rc-util/lib/warning';
31
import toNodeArray from 'rc-util/lib/Children/toArray';
4-
import { convertChildrenToData } from './legacyUtil';
5-
import { toArray } from './commonUtil';
6-
import type { RawValueType, LabelInValueType, BaseOptionType, SelectProps } from '../Select';
2+
import warning, { noteOnce } from 'rc-util/lib/warning';
3+
import * as React from 'react';
74
import { isMultiple } from '../BaseSelect';
5+
import type {
6+
BaseOptionType,
7+
DefaultOptionType,
8+
FieldNames,
9+
LabelInValueType,
10+
RawValueType,
11+
SelectProps,
12+
} from '../Select';
13+
import { toArray } from './commonUtil';
14+
import { convertChildrenToData } from './legacyUtil';
815

916
function warningProps(props: SelectProps) {
1017
const {
@@ -150,4 +157,31 @@ function warningProps(props: SelectProps) {
150157
}
151158
}
152159

160+
// value in Select option should not be null
161+
// note: OptGroup has options too
162+
export function warningNullOptions(options: DefaultOptionType[], fieldNames: FieldNames) {
163+
if (options) {
164+
const recursiveOptions = (optionsList: DefaultOptionType[], inGroup: boolean = false) => {
165+
for (let i = 0; i < optionsList.length; i++) {
166+
const option = optionsList[i];
167+
168+
if (option[fieldNames?.value] === null) {
169+
warning(false, '`value` in Select options should not be `null`.');
170+
return true;
171+
}
172+
173+
if (
174+
!inGroup &&
175+
Array.isArray(option[fieldNames?.options]) &&
176+
recursiveOptions(option[fieldNames?.options], true)
177+
) {
178+
break;
179+
}
180+
}
181+
};
182+
183+
recursiveOptions(options);
184+
}
185+
}
186+
153187
export default warningProps;

‎tests/Select.test.tsx‎

Lines changed: 96 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -1,27 +1,27 @@
11
import { mount, render } from 'enzyme';
22
import KeyCode from 'rc-util/lib/KeyCode';
3-
import React from 'react';
4-
import { act } from 'react-dom/test-utils';
5-
import { resetWarned } from 'rc-util/lib/warning';
6-
import type { ScrollConfig } from 'rc-virtual-list/lib/List';
73
import { spyElementPrototype } from 'rc-util/lib/test/domHook';
4+
import { resetWarned } from 'rc-util/lib/warning';
85
import VirtualList from 'rc-virtual-list';
6+
import type { ScrollConfig } from 'rc-virtual-list/lib/List';
7+
import React from 'react';
8+
import { act } from 'react-dom/test-utils';
99
import type { SelectProps } from '../src';
1010
import Select, { OptGroup, Option, useBaseProps } from '../src';
11-
import focusTest from './shared/focusTest';
11+
import type { BaseSelectRef } from '../src/BaseSelect';
12+
import allowClearTest from './shared/allowClearTest';
1213
import blurTest from './shared/blurTest';
13-
import keyDownTest from './shared/keyDownTest';
14+
import focusTest from './shared/focusTest';
1415
import inputFilterTest from './shared/inputFilterTest';
16+
import keyDownTest from './shared/keyDownTest';
1517
import openControlledTest from './shared/openControlledTest';
16-
import allowClearTest from './shared/allowClearTest';
1718
import {
1819
expectOpen,
19-
toggleOpen,
20-
selectItem,
2120
findSelection,
2221
injectRunAllTimers,
22+
selectItem,
23+
toggleOpen,
2324
} from './utils/common';
24-
import type { BaseSelectRef } from '../src/BaseSelect';
2525

2626
describe('Select.Basic', () => {
2727
injectRunAllTimers(jest);
@@ -602,7 +602,7 @@ describe('Select.Basic', () => {
602602
</Select>,
603603
);
604604

605-
const inputSpy = jest.spyOn(wrapper2.find('input').instance(), 'focus');
605+
const inputSpy = jest.spyOn(wrapper2.find('input').instance(), 'focus' as any);
606606

607607
wrapper2.find('.rc-select-selection-placeholder').simulate('mousedown');
608608
wrapper2.find('.rc-select-selection-placeholder').simulate('click');
@@ -807,9 +807,9 @@ describe('Select.Basic', () => {
807807
}
808808
}
809809
const wrapper = mount(<Controlled />);
810-
expect(wrapper.state().open).toBe(true);
810+
expect((wrapper.state() as any).open).toBe(true);
811811
toggleOpen(wrapper);
812-
expect(wrapper.state().open).toBe(false);
812+
expect((wrapper.state() as any).open).toBe(false);
813813

814814
selectItem(wrapper);
815815
expectOpen(wrapper, false);
@@ -822,7 +822,7 @@ describe('Select.Basic', () => {
822822
</Select>,
823823
);
824824

825-
const focusSpy = jest.spyOn(wrapper.find('input').instance(), 'focus');
825+
const focusSpy = jest.spyOn(wrapper.find('input').instance(), 'focus' as any);
826826
wrapper.find('.rc-select-selection-placeholder').simulate('mousedown');
827827
wrapper.find('.rc-select-selection-placeholder').simulate('click');
828828
expect(focusSpy).toHaveBeenCalled();
@@ -1646,28 +1646,90 @@ describe('Select.Basic', () => {
16461646
});
16471647
});
16481648

1649-
it('`null` is a value', () => {
1650-
const onChange = jest.fn();
1649+
describe('`null` is a value', () => {
1650+
let errorSpy;
1651+
const warningMessage = 'Warning: `value` in Select options should not be `null`.';
16511652

1652-
const wrapper = mount(
1653-
<Select onChange={onChange}>
1654-
<Option value={1}>1</Option>
1655-
<Option value={null}>No</Option>
1656-
<Option value={0}>0</Option>
1657-
<Option value="">Empty</Option>
1658-
</Select>,
1659-
);
1653+
beforeAll(() => {
1654+
errorSpy = jest.spyOn(console, 'error').mockImplementation(() => null);
1655+
});
16601656

1661-
[
1662-
[1, '1'],
1663-
[null, 'No'],
1664-
[0, '0'],
1665-
['', 'Empty'],
1666-
].forEach(([value, showValue], index) => {
1667-
toggleOpen(wrapper);
1668-
selectItem(wrapper, index);
1669-
expect(onChange).toHaveBeenCalledWith(value, expect.anything());
1670-
expect(wrapper.find('.rc-select-selection-item').text()).toEqual(showValue);
1657+
beforeEach(() => {
1658+
errorSpy.mockReset();
1659+
resetWarned();
1660+
});
1661+
1662+
afterAll(() => {
1663+
errorSpy.mockRestore();
1664+
});
1665+
1666+
it('`null` is a value and should throw a warning', () => {
1667+
const onChange = jest.fn();
1668+
1669+
const wrapper = mount(
1670+
<Select onChange={onChange}>
1671+
<Option value={1}>1</Option>
1672+
<Option value={null}>No</Option>
1673+
<Option value={0}>0</Option>
1674+
<Option value="">Empty</Option>
1675+
</Select>,
1676+
);
1677+
1678+
[
1679+
[1, '1'],
1680+
[null, 'No'],
1681+
[0, '0'],
1682+
['', 'Empty'],
1683+
].forEach(([value, showValue], index) => {
1684+
toggleOpen(wrapper);
1685+
selectItem(wrapper, index);
1686+
expect(onChange).toHaveBeenCalledWith(value, expect.anything());
1687+
expect(wrapper.find('.rc-select-selection-item').text()).toEqual(showValue);
1688+
});
1689+
1690+
expect(errorSpy).toHaveBeenCalledWith(warningMessage);
1691+
});
1692+
1693+
it('`null` is a value in OptGroup should throw a warning', () => {
1694+
mount(
1695+
<Select>
1696+
<OptGroup>
1697+
<Option value="1">1</Option>
1698+
<Option value={null}>null</Option>
1699+
</OptGroup>
1700+
</Select>,
1701+
);
1702+
1703+
expect(errorSpy).toHaveBeenCalledWith(warningMessage);
1704+
});
1705+
1706+
it('`null` is a value in fieldNames should throw a warning', () => {
1707+
mount(
1708+
<Select
1709+
fieldNames={{
1710+
label: 'fieldLabel',
1711+
value: 'fieldValue',
1712+
options: 'fieldOptions',
1713+
}}
1714+
options={[
1715+
{
1716+
fieldLabel: 'label',
1717+
fieldOptions: [
1718+
{
1719+
fieldLabel: '1',
1720+
fieldValue: '1',
1721+
},
1722+
{
1723+
fieldLabel: '2',
1724+
fieldValue: null,
1725+
},
1726+
],
1727+
},
1728+
]}
1729+
/>,
1730+
);
1731+
1732+
expect(errorSpy).toHaveBeenCalledWith(warningMessage);
16711733
});
16721734
});
16731735

0 commit comments

Comments
 (0)