fix(Picky): Stop using internal state

Internal state was flaky to manage, there wasn't a real benefit to being an uncontrolled component since you needed to provide onChange and a value anyway. A couple of bugs have been caused due to this. Better just to remove it. BREAKING CHANGE: No more uncontrolled components. Use onChange and value, set the value from state and update the state from onChange.
Aidurber · Oct 18, 2018 · 22db05f · 22db05f
1 parent a5b6661
commit 22db05f
Show file tree

Hide file tree

Showing 7 changed files with 69 additions and 108 deletions.
diff --git a/README.md b/README.md
@@ -137,7 +137,7 @@ Picky.propTypes = {
 - `numberDisplayed` - Then number of selected options displayed until it turns into '(selected count) selected'.
 - `multiple` - Set to true for a multiselect, defaults to false.
 - `options` - Array of possible options.
-- `onChange` - Called whenever selected value(s) have changed. If you are using this as a controlled component, pass the selected value back into `value`.
+- `onChange` - Called whenever selected value(s) have changed. Pass the selected value back into `value`.
 - `open` - Can open or close the dropdown manually. Defaults to false.
 - `includeSelectAll` - If set to `true` will add a `Select All` checkbox at the top of the list.
 - `includeFilter` - If set to `true` will add an input at the top of the dropdown for filtering the results.

diff --git a/dist/index.js b/dist/index.js
diff --git a/docs/README.md b/docs/README.md
@@ -25,6 +25,8 @@ This is a multiselect with checkboxes, a select all option, and a filter. Along
 
 If you like the tag list like [React-Select](https://github.com/JedWatson/react-select), then that would be a great option for you. It's a really great, well-tested library. Give it a look.
 
+You can also achieve the same result with a great deal of flexibility using [Paypal's Downshift](https://github.com/paypal/downshift#usage).
+
 # Peer Dependencies
 
 ```
@@ -135,7 +137,7 @@ Picky.propTypes = {
 - `numberDisplayed` - Then number of selected options displayed until it turns into '(selected count) selected'.
 - `multiple` - Set to true for a multiselect, defaults to false.
 - `options` - Array of possible options.
-- `onChange` - Called whenever selected value(s) have changed. If you are using this as a controlled component, pass the selected value back into `value`.
+- `onChange` - Called whenever selected value(s) have changed. Pass the selected value back into `value`.
 - `open` - Can open or close the dropdown manually. Defaults to false.
 - `includeSelectAll` - If set to `true` will add a `Select All` checkbox at the top of the list.
 - `includeFilter` - If set to `true` will add an input at the top of the dropdown for filtering the results.

diff --git a/src/Filter.js b/src/Filter.js
@@ -1,25 +1,6 @@
-import React, { Component } from 'react';
+import React, { PureComponent } from 'react';
 import PropTypes from 'prop-types';
-class Filter extends Component {
-  constructor(props) {
-    super(props);
-    this.state = {
-      filterTerm: '',
-    };
-
-    this.onFilterChange = this.onFilterChange.bind(this);
-  }
-  onFilterChange(event) {
-    const query = event.target.value;
-    this.setState(
-      {
-        filterTerm: query,
-      },
-      () => {
-        this.props.onFilterChange(query);
-      }
-    );
-  }
+class Filter extends PureComponent {
   render() {
     return (
       <div className="picky__filter">
@@ -31,8 +12,7 @@ class Filter extends Component {
           placeholder="Filter..."
           tabIndex={this.props.tabIndex}
           aria-label="filter options"
-          value={this.state.filterTerm}
-          onChange={this.onFilterChange}
+          onChange={e => this.props.onFilterChange(e.target.value)}
         />
       </div>
     );

diff --git a/src/Picky.js b/src/Picky.js
@@ -50,25 +50,23 @@ class Picky extends React.PureComponent {
   UNSAFE_componentWillReceiveProps(nextProps) {
     if (
       this.props.options !== nextProps.options ||
-      this.state.selectedValue !== nextProps.value
+      this.props.value !== nextProps.value
     ) {
       this.setState({
-        allSelected: this.allSelected(),
-      });
-    }
-
-    if (!isEqual(nextProps.value, this.state.selectedValue)) {
-      this.setState({
-        selectedValue: nextProps.value,
-        allSelected: this.allSelected(nextProps.value),
+        allSelected: !isEqual(nextProps.value, this.props.value)
+          ? this.allSelected(nextProps.value)
+          : this.allSelected(),
       });
     }
   }
 
   selectValue(val) {
-    const valueLookup = this.isControlled()
-      ? this.props.value
-      : this.state.selectedValue;
+    const valueLookup = this.props.value;
+    const isObject = isDataObject(
+      val,
+      this.props.valueKey,
+      this.props.labelKey
+    );
     if (this.props.multiple && Array.isArray(valueLookup)) {
       const itemIndex = hasItemIndex(
         valueLookup,
@@ -84,26 +82,26 @@ class Picky extends React.PureComponent {
           ...valueLookup.slice(itemIndex + 1),
         ];
       } else {
-        selectedValue = [...this.state.selectedValue, val];
+        let ids = valueLookup;
+        const item = isObject ? val[this.props.valueKey] : val;
+        if (isObject) {
+          ids = valueLookup.map(value => value[this.props.valueKey]);
+        }
+        selectedValue =
+          ids.indexOf(item) === -1
+            ? [...this.props.value, val]
+            : [...this.props.value];
       }
       this.setState(
         {
-          selectedValue,
           allSelected: this.allSelected(selectedValue),
         },
         () => {
-          this.props.onChange(this.state.selectedValue);
+          this.props.onChange(selectedValue);
         }
       );
     } else {
-      this.setState(
-        {
-          selectedValue: val,
-        },
-        () => {
-          this.props.onChange(this.state.selectedValue);
-        }
-      );
+      this.props.onChange(val);
     }
   }
 
@@ -114,7 +112,7 @@ class Picky extends React.PureComponent {
    * @memberof Picky
    */
   allSelected(overrideSelected) {
-    const selectedValue = overrideSelected || this.state.selectedValue;
+    const selectedValue = overrideSelected || this.props.value;
     const { options } = this.props;
     const copiedOptions = options.slice(0);
     const copiedSelectedValue = Array.isArray(selectedValue)
@@ -131,30 +129,25 @@ class Picky extends React.PureComponent {
   toggleSelectAll() {
     this.setState(
       {
-        selectedValue: !this.state.allSelected ? this.props.options : [],
         allSelected: !this.state.allSelected,
       },
       () => {
-        // Call onChange prop with new values
-        this.props.onChange(this.state.selectedValue);
+        if (!this.state.allSelected) {
+          this.props.onChange([]);
+        } else {
+          this.props.onChange(this.props.options);
+        }
       }
     );
   }
-  /**
-   * Determine whether the user is treating this as a controlled component or not.
-   *
-   * @returns
-   * @memberof Picky
-   */
-  isControlled() {
-    return !!this.props.value;
-  }
 
   isItemSelected(item) {
-    const value = this.isControlled()
-      ? this.props.value
-      : this.state.selectedValue;
-    return hasItem(value, item, this.props.valueKey, this.props.labelKey);
+    return hasItem(
+      this.props.value,
+      item,
+      this.props.valueKey,
+      this.props.labelKey
+    );
   }
 
   /**
@@ -176,7 +169,7 @@ class Picky extends React.PureComponent {
     if (renderList) {
       return renderList({
         items,
-        selected: this.state.selectedValue,
+        selected: this.props.value,
         multiple,
         tabIndex,
         getIsSelected: this.isItemSelected,
@@ -360,7 +353,7 @@ class Picky extends React.PureComponent {
             placeholder={placeholder}
             manySelectedPlaceholder={this.props.manySelectedPlaceholder}
             allSelectedPlaceholder={this.props.allSelectedPlaceholder}
-            value={this.isControlled() ? value : this.state.selectedValue}
+            value={value}
             multiple={multiple}
             numberDisplayed={numberDisplayed}
             valueKey={valueKey}

diff --git a/tests/Picky.test.js b/tests/Picky.test.js
@@ -13,11 +13,6 @@ const selSelected = id => `[data-testid="${id}"][data-selected="selected"]`;
 describe('Picky', () => {
   afterEach(cleanup); // <-- add this
 
-  it('should select initial values on load', () => {
-    const wrapper = mount(<Picky value={[1, 2, 3]} multiple />);
-    expect(wrapper.state('selectedValue')).toEqual([1, 2, 3]);
-  });
-
   it('should have Placeholder component', () => {
     const wrapper = mount(<Picky value={[]} />);
     expect(wrapper.find(Placeholder)).toHaveLength(1);
@@ -328,11 +323,12 @@ describe('Picky', () => {
 
     it('should select all options when select all is clicked', () => {
       const onChange = jest.fn();
+      const options = [1, 2, 3, 4, 5];
       const wrapper = mount(
         <Picky
           value={[1, 2, 3]}
           includeSelectAll={true}
-          options={[1, 2, 3, 4, 5]}
+          options={options}
           open={true}
           multiple={true}
           onChange={onChange}
@@ -342,11 +338,9 @@ describe('Picky', () => {
 
       expect(wrapper.find(selSelected('option'))).toHaveLength(3);
       selectAllItem.simulate('click');
-      expect(wrapper.state('selectedValue')).toHaveLength(5);
+      expect(onChange).toHaveBeenLastCalledWith(options);
       selectAllItem.simulate('click');
-      expect(wrapper.state('selectedValue')).toHaveLength(0);
-      expect(onChange).toHaveBeenCalled();
-      expect(onChange).toHaveBeenCalledWith([1, 2, 3, 4, 5]);
+      expect(onChange).toHaveBeenLastCalledWith([]);
     });
 
     it('should select single value controlled', () => {
@@ -364,7 +358,7 @@ describe('Picky', () => {
         .find(sel('option'))
         .at(1)
         .simulate('click');
-      expect(onChange).lastCalledWith(2);
+      expect(onChange).toHaveBeenLastCalledWith(2);
     });
 
     it('should select single value uncontrolled', () => {
@@ -377,7 +371,7 @@ describe('Picky', () => {
         .find(sel('option'))
         .at(1)
         .simulate('click');
-      expect(onChange).lastCalledWith(2);
+      expect(onChange).toHaveBeenLastCalledWith(2);
     });
     it('should select multiple value controlled', () => {
       const onChange = jest.fn();
@@ -391,32 +385,11 @@ describe('Picky', () => {
         />
       );
 
-      expect(wrapper.state('selectedValue')).toEqual([]);
       wrapper
         .find(sel('option'))
         .at(1)
         .simulate('click');
-      expect(onChange).lastCalledWith([2]);
-      expect(wrapper.state('selectedValue')).toEqual([2]);
-    });
-    it('should select multiple value uncontrolled', () => {
-      const onChange = jest.fn();
-      const wrapper = mount(
-        <Picky
-          options={[1, 2, 3, 4, 5]}
-          open={true}
-          multiple
-          onChange={onChange}
-        />
-      );
-
-      expect(wrapper.state('selectedValue')).toEqual([]);
-      wrapper
-        .find(sel('option'))
-        .at(1)
-        .simulate('click');
-      expect(onChange).lastCalledWith([2]);
-      expect(wrapper.state('selectedValue')).toEqual([2]);
+      expect(onChange).toHaveBeenLastCalledWith([2]);
     });
 
     it('should deselect multiple value', () => {
@@ -431,13 +404,11 @@ describe('Picky', () => {
         />
       );
 
-      expect(wrapper.state('selectedValue')).toEqual([2]);
       wrapper
         .find(sel('option'))
         .at(1)
         .simulate('click');
-      expect(onChange).lastCalledWith([]);
-      expect(wrapper.state('selectedValue')).toEqual([]);
+      expect(onChange).toHaveBeenLastCalledWith([]);
     });
 
     it('should support object options single select', () => {
@@ -446,13 +417,15 @@ describe('Picky', () => {
         { id: 2, name: 'Item 2' },
         { id: 3, name: 'Item 3' },
       ];
+      const mockOnChange = jest.fn();
       const wrapper = mount(
         <Picky
           value={null}
           options={options}
           open={true}
           valueKey="id"
           labelKey="name"
+          onChange={mockOnChange}
         />
       );
 
@@ -461,7 +434,7 @@ describe('Picky', () => {
         .at(0)
         .simulate('click');
 
-      expect(wrapper.state('selectedValue')).toEqual({
+      expect(mockOnChange).toHaveBeenLastCalledWith({
         id: 1,
         name: 'Item 1',
       });
@@ -713,7 +686,16 @@ describe('Picky', () => {
 
     describe('Issue #38', () => {
       test('should set selectedValue state when async value prop updates', done => {
-        const wrapper = mount(<Picky multiple open value={[]} options={[]} />);
+        const mockOnChange = jest.fn();
+        const wrapper = mount(
+          <Picky
+            multiple
+            open
+            value={[]}
+            options={[]}
+            onChange={mockOnChange}
+          />
+        );
         const componentWillUpdateSpy = jest.spyOn(
           Picky.prototype,
           'UNSAFE_componentWillReceiveProps'
@@ -725,7 +707,6 @@ describe('Picky', () => {
             options: ['1', '2', '3', '4', '5'],
           });
           expect(componentWillUpdateSpy).toHaveBeenCalled();
-          expect(wrapper.state('selectedValue')).toHaveLength(2);
           expect(wrapper.state('allSelected')).toEqual(false);
           componentWillUpdateSpy.mockReset();
           componentWillUpdateSpy.mockRestore();

diff --git a/yarn.lock b/yarn.lock
@@ -1410,6 +1410,11 @@ babel-plugin-transform-property-literals@^6.9.4:
   dependencies:
     esutils "^2.0.2"
 
+babel-plugin-transform-react-remove-prop-types@^0.4.19:
+  version "0.4.19"
+  resolved "https://registry.yarnpkg.com/babel-plugin-transform-react-remove-prop-types/-/babel-plugin-transform-react-remove-prop-types-0.4.19.tgz#dc9d8fb176a407a75efe73f231550450e29a3b17"
+  integrity sha512-f49NsaohQ1ByY20nUrpc30QFdbeT4ntV4PAL2vSZe6uCB5nqAcqXS/qzU+aI6ZfYhWASx5eIsTFvFrs1B2ffGg==
+
 babel-plugin-transform-regexp-constructors@^0.4.3:
   version "0.4.3"
   resolved "https://registry.yarnpkg.com/babel-plugin-transform-regexp-constructors/-/babel-plugin-transform-regexp-constructors-0.4.3.tgz#58b7775b63afcf33328fae9a5f88fbd4fb0b4965"