Skip to content

feat: Support legacy props param with warning #424

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 2 commits into from
Sep 17, 2019
Merged

feat: Support legacy props param with warning #424

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
cc86c00
add warning
zombieJ Sep 17, 2019
04bdf68
add warning if use `props`
zombieJ Sep 17, 2019
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
30 changes: 23 additions & 7 deletions src/utils/valueUtil.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,26 @@ export function flattenOptions(options: SelectOptionsType): FlattenOptionData[]
return flattenList;
}

/**
* Inject `props` into `option` for legacy usage
*/
function injectPropsWithOption<T>(option: T): T {
const newOption = { ...option };
if (!('props' in newOption)) {
Object.defineProperty(newOption, 'props', {
get() {
warning(
false,
'Return type is option instead of Option instance. Please read value directly instead of reading from `props`.',
);
return newOption;
},
});
}

return newOption;
}

export function findValueOption(
values: RawValueType[],
options: FlattenOptionData[],
Expand All @@ -81,7 +101,7 @@ export function findValueOption(
}
});

return values.map(val => optionMap.get(val));
return values.map(val => injectPropsWithOption(optionMap.get(val)));
}

export const getLabeledValue: GetLabeledValue<FlattenOptionData[]> = (
Expand Down Expand Up @@ -193,7 +213,7 @@ export function filterOptions(
return;
}

if (filterFunc(searchValue, item)) {
if (filterFunc(searchValue, injectPropsWithOption(item))) {
filteredOptions.push(item);
}
});
Expand Down Expand Up @@ -227,11 +247,7 @@ export function getSeparatedContent(text: string, tokens: string[]): string[] {

export function isValueDisabled(value: RawValueType, options: FlattenOptionData[]): boolean {
const option = findValueOption([value], options)[0];
if (option) {
return option.disabled;
}

return false;
return option.disabled;
}

/**
Expand Down
39 changes: 16 additions & 23 deletions tests/Combobox.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,12 +6,7 @@ import Select, { Option, SelectProps } from '../src';
import focusTest from './shared/focusTest';
import keyDownTest from './shared/keyDownTest';
import openControlledTest from './shared/openControlledTest';
import {
expectOpen,
toggleOpen,
selectItem,
injectRunAllTimers,
} from './utils/common';
import { expectOpen, toggleOpen, selectItem, injectRunAllTimers } from './utils/common';
import allowClearTest from './shared/allowClearTest';
import throwOptionValue from './shared/throwOptionValue';

Expand Down Expand Up @@ -71,10 +66,10 @@ describe('Select.Combobox', () => {
);

wrapper.find('input').simulate('change', { target: { value: '1' } });
expect(handleChange).toBeCalledWith('1', undefined);
expect(handleChange).toHaveBeenCalledWith('1', {});

wrapper.find('input').simulate('change', { target: { value: '22' } });
expect(handleChange).toBeCalledWith(
expect(handleChange).toHaveBeenCalledWith(
'22',
expect.objectContaining({
children: '22',
Expand Down Expand Up @@ -134,13 +129,15 @@ describe('Select.Combobox', () => {
public state = {
data: [],
};

public handleChange = () => {
setTimeout(() => {
this.setState({
data: [{ key: '1', label: '1' }, { key: '2', label: '2' }],
});
}, 500);
};

public render() {
const options = this.state.data.map(item => <Option key={item.key}>{item.label}</Option>);
return (
Expand Down Expand Up @@ -171,13 +168,15 @@ describe('Select.Combobox', () => {
public state = {
data: [{ key: '1', label: '1' }, { key: '2', label: '2' }],
};

public onSelect = () => {
setTimeout(() => {
this.setState({
data: [{ key: '3', label: '3' }, { key: '4', label: '4' }],
});
}, 500);
};

public render() {
const options = this.state.data.map(item => <Option key={item.key}>{item.label}</Option>);
return (
Expand Down Expand Up @@ -208,27 +207,21 @@ describe('Select.Combobox', () => {
const handleChange = jest.fn();
const handleSelect = jest.fn();
const wrapper = mount(
<Select
mode="combobox"
backfill={true}
open={true}
onChange={handleChange}
onSelect={handleSelect}
>
<Select mode="combobox" backfill open onChange={handleChange} onSelect={handleSelect}>
<Option value="One">One</Option>
<Option value="Two">Two</Option>
</Select>,
);
const input = wrapper.find('input');
input.simulate('keyDown', { which: KeyCode.DOWN });
expect(wrapper.find('input').props().value).toEqual('One');
expect(handleChange).not.toBeCalled();
expect(handleSelect).not.toBeCalled();
expect(handleChange).not.toHaveBeenCalled();
expect(handleSelect).not.toHaveBeenCalled();

input.simulate('keyDown', { which: KeyCode.ENTER });
expect(wrapper.find('input').props().value).toEqual('One');
expect(handleChange).toBeCalledWith('One', expect.objectContaining({ value: 'One' }));
expect(handleSelect).toBeCalledWith('One', expect.objectContaining({ value: 'One' }));
expect(handleChange).toHaveBeenCalledWith('One', expect.objectContaining({ value: 'One' }));
expect(handleSelect).toHaveBeenCalledWith('One', expect.objectContaining({ value: 'One' }));
});

it("should hide clear icon when value is ''", () => {
Expand Down Expand Up @@ -283,9 +276,9 @@ describe('Select.Combobox', () => {
public render() {
return (
<Select mode="combobox" onChange={this.updateOptions}>
{this.state.options.map(opt => {
return <Option key={opt}>{opt}</Option>;
})}
{this.state.options.map(opt => (
<Option key={opt}>{opt}</Option>
))}
</Select>
);
}
Expand Down Expand Up @@ -328,7 +321,7 @@ describe('Select.Combobox', () => {
});
jest.runAllTimers();
wrapper.update();
expect(onDropdownVisibleChange).toBeCalledWith(false);
expect(onDropdownVisibleChange).toHaveBeenCalledWith(false);
jest.useRealTimers();
});
});
83 changes: 83 additions & 0 deletions tests/Select.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1217,4 +1217,87 @@ describe('Select.Basic', () => {
);
errorSpy.mockRestore();
});

describe('warning if use `props` to read data', () => {
it('filterOption', () => {
resetWarned();
const errorSpy = jest.spyOn(console, 'error').mockImplementation(() => {});

const wrapper = mount(
<Select
filterOption={(input, option) =>
option.props.children.toLowerCase().indexOf(input.toLowerCase()) >= 0
}
showSearch
>
<Option value="light">Light</Option>
<Option value="bamboo">Bamboo</Option>
</Select>,
);

wrapper.find('input').simulate('change', { target: { value: 'l' } });
expect(wrapper.find('List').props().data).toHaveLength(1);
expect(wrapper.find('div.rc-select-item-option-content').text()).toBe('Light');

expect(errorSpy).toHaveBeenCalledWith(
'Warning: Return type is option instead of Option instance. Please read value directly instead of reading from `props`.',
);
errorSpy.mockRestore();
});

it('Select & Deselect', () => {
resetWarned();
const errorSpy = jest.spyOn(console, 'error').mockImplementation(() => {});

const readPropsFunc = (_, opt) => {
expect(opt.props).toBeTruthy();
};

const wrapper = mount(
<Select mode="multiple" onSelect={readPropsFunc} onDeselect={readPropsFunc}>
<Option value="light">Light</Option>
<Option value="bamboo">Bamboo</Option>
</Select>,
);

toggleOpen(wrapper);
selectItem(wrapper);
expect(errorSpy).toHaveBeenCalledWith(
'Warning: Return type is option instead of Option instance. Please read value directly instead of reading from `props`.',
);

errorSpy.mockReset();
resetWarned();
selectItem(wrapper);
expect(errorSpy).toHaveBeenCalledWith(
'Warning: Return type is option instead of Option instance. Please read value directly instead of reading from `props`.',
);

errorSpy.mockRestore();
});

it('onChange', () => {
resetWarned();
const errorSpy = jest.spyOn(console, 'error').mockImplementation(() => {});

const readPropsFunc = (_, opt) => {
expect(opt.props).toBeTruthy();
};

const wrapper = mount(
<Select onChange={readPropsFunc}>
<Option value="light">Light</Option>
<Option value="bamboo">Bamboo</Option>
</Select>,
);

toggleOpen(wrapper);
selectItem(wrapper);
expect(errorSpy).toHaveBeenCalledWith(
'Warning: Return type is option instead of Option instance. Please read value directly instead of reading from `props`.',
);

errorSpy.mockRestore();
});
});
});
10 changes: 5 additions & 5 deletions tests/Tags.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ describe('Select.Tags', () => {

jest.runAllTimers();
expect(findSelection(wrapper).text()).toBe('foo');
expect(onChange).toHaveBeenCalledWith(['foo'], [undefined]);
expect(onChange).toHaveBeenCalledWith(['foo'], [{}]);
});

it('tokenize input', () => {
Expand All @@ -76,9 +76,9 @@ describe('Select.Tags', () => {

wrapper.find('input').simulate('change', { target: { value: '2,3,4' } });

expect(handleChange).toBeCalledWith(['2', '3', '4'], expect.anything());
expect(handleChange).toHaveBeenCalledWith(['2', '3', '4'], expect.anything());
expect(handleSelect).toHaveBeenCalledTimes(3);
expect(handleSelect).toHaveBeenLastCalledWith('4', undefined);
expect(handleSelect).toHaveBeenLastCalledWith('4', expect.anything());
expect(findSelection(wrapper).text()).toEqual('2');
expect(findSelection(wrapper, 1).text()).toEqual('3');
expect(findSelection(wrapper, 2).text()).toEqual('4');
Expand Down Expand Up @@ -187,7 +187,7 @@ describe('Select.Tags', () => {

it('should work fine when filterOption function exists', () => {
const children = [];
for (let i = 10; i < 36; i++) {
for (let i = 10; i < 36; i += 1) {
children.push(
<Option key={i.toString(36) + i} disabled={!(i % 3)}>
{i.toString(36) + i}
Expand All @@ -199,7 +199,7 @@ describe('Select.Tags', () => {
mode="tags"
style={{ width: '100%' }}
placeholder="Tags Mode"
filterOption={(input, { key }) => key.indexOf(input) >= 0}
filterOption={(input, { key }) => String(key).indexOf(input) >= 0}
>
{children}
</Select>,
Expand Down
12 changes: 6 additions & 6 deletions tests/shared/dynamicChildrenTest.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ export default function dynamicChildrenTest(mode: any, props?: Partial<SelectPro
<Option key="2">2-label</Option>,
],
};

public select: any;

public componentDidMount() {
Expand Down Expand Up @@ -65,14 +66,11 @@ export default function dynamicChildrenTest(mode: any, props?: Partial<SelectPro
wrapper.update();
toggleOpen(wrapper);
selectItem(wrapper, 1);
expect(onChange).toBeCalledWith(
expect(onChange).toHaveBeenCalledWith(
['1', '3'],
[
mode === 'tags' ? expect.anything() : undefined,
expect.objectContaining({ value: '3', children: '3-label' }),
],
[expect.anything(), expect.objectContaining({ value: '3', children: '3-label' })],
);
expect(onSelect).toBeCalledWith(
expect(onSelect).toHaveBeenCalledWith(
'3',
expect.objectContaining({ value: '3', children: '3-label' }),
);
Expand All @@ -91,6 +89,7 @@ export default function dynamicChildrenTest(mode: any, props?: Partial<SelectPro
</Option>,
],
};

public select: any;

public componentDidMount() {
Expand Down Expand Up @@ -147,6 +146,7 @@ export default function dynamicChildrenTest(mode: any, props?: Partial<SelectPro
<Option key="2">2-label</Option>,
],
};

public select: any;

public componentDidMount() {
Expand Down