fix: Compare Options with getOptionKey instead of label + reduce (#1012)

src/components/Select.vue

@@ -84,6 +84,7 @@
   import ajax from '../mixins/ajax'
   import childComponents from './childComponents';
   import appendToBody from '../directives/appendToBody';
+  import sortAndStringify from '../utility/sortAndStringify'
   import uniqueId from '../utility/uniqueId';
 
   /**
@@ -281,12 +282,16 @@
       },
 
       /**
-       * Callback to get an option key. If {option}
-       * is an object and has an {id}, returns {option.id}
-       * by default, otherwise tries to serialize {option}
-       * to JSON.
+       * Generate a unique identifier for each option. If `option`
+       * is an object and `option.hasOwnProperty('id')` exists,
+       * `option.id` is used by default, otherwise the option
+       * will be serialized to JSON.
        *
-       * The key must be unique for an option.
+       * If you are supplying a lot of options, you should
+       * provide your own keys, as JSON.stringify can be
+       * slow with lots of objects.
+       *
+       * The result of this function *must* be unique.
        *
        * @type {Function}
        * @param  {Object || String} option
@@ -294,22 +299,21 @@
        */
       getOptionKey: {
         type: Function,
-        default(option) {
-          if (typeof option === 'object' && option.id) {
-            return option.id
-          } else {
-            try {
-              return JSON.stringify(option)
-            } catch(e) {
-              return console.warn(
-                `[vue-select warn]: Could not stringify option ` +
-                `to generate unique key. Please provide'getOptionKey' prop ` +
-                `to return a unique key for each option.\n` +
-                'https://vue-select.org/api/props.html#getoptionkey'
-              );
-            }
+        default (option) {
+          if (typeof option !== 'object') {
+            return option;
           }
-        }
+
+          try {
+            return option.hasOwnProperty('id') ? option.id : sortAndStringify(option);
+          } catch (e) {
+            const warning = `[vue-select warn]: Could not stringify this option ` +
+              `to generate unique key. Please provide'getOptionKey' prop ` +
+              `to return a unique key for each option.\n` +
+              'https://vue-select.org/api/props.html#getoptionkey';
+            return console.warn(warning, option, e);
+          }
+        },
       },
 
       /**
@@ -393,7 +397,7 @@
        * @return {Boolean}
        */
       filter: {
-        "type": Function,
+        type: Function,
         default(options, search) {
           return options.filter((option) => {
             let label = this.getOptionLabel(option)
@@ -643,7 +647,6 @@
       select(option) {
         if (!this.isOptionSelected(option)) {
           if (this.taggable && !this.optionExists(option)) {
-            option = this.createOption(option);
             this.$emit('option:created', option);
           }
           if (this.multiple) {
@@ -746,38 +749,18 @@
        * @return {Boolean}        True when selected | False otherwise
        */
       isOptionSelected(option) {
-        return this.selectedValue.some(value => {
-          return this.optionComparator(value, option)
-        })
+        return this.selectedValue.some(value => this.optionComparator(value, option))
       },
 
       /**
        * Determine if two option objects are matching.
        *
-       * @param value {Object}
-       * @param option {Object}
+       * @param a {Object}
+       * @param b {Object}
        * @returns {boolean}
        */
-      optionComparator(value, option) {
-        if (typeof value !== 'object' && typeof option !== 'object') {
-          // Comparing primitives
-          if (value === option) {
-            return true
-          }
-        } else {
-          // Comparing objects
-          if (value === this.reduce(option)) {
-            return true
-          }
-          if ((this.getOptionLabel(value) === this.getOptionLabel(option)) || (this.getOptionLabel(value) === option)) {
-            return true
-          }
-          if (this.reduce(value) === this.reduce(option)) {
-            return true
-          }
-        }
-
-        return false;
+      optionComparator(a, b) {
+        return this.getOptionKey(a) === this.getOptionKey(b);
       },
 
       /**
@@ -825,14 +808,7 @@
        * @return {boolean}
        */
       optionExists(option) {
-        return this.optionList.some(opt => {
-          if (typeof opt === 'object' && this.getOptionLabel(opt) === option) {
-            return true
-          } else if (opt === option) {
-            return true
-          }
-          return false
-        })
+        return this.optionList.some(_option => this.optionComparator(_option, option))
       },
 
       /**
@@ -1138,7 +1114,7 @@
         }
 
         let options = this.search.length ? this.filter(optionList, this.search, this) : optionList;
-        if (this.taggable && this.search.length && !this.optionExists(this.search)) {
+        if (this.taggable && this.search.length && !this.optionExists(this.createOption(this.search))) {
           options.unshift(this.search)
         }
         return options
@@ -1158,7 +1134,7 @@
        */
       showClearButton() {
         return !this.multiple && this.clearable && !this.open && !this.isValueEmpty
-      }
+      },
     },
 
   }

src/mixins/typeAheadPointer.js

@@ -56,15 +56,15 @@ export default {
      * Optionally clear the search input on selection.
      * @return {void}
      */
-    typeAheadSelect() {
-      if( this.filteredOptions[ this.typeAheadPointer ] ) {
-        this.select( this.filteredOptions[ this.typeAheadPointer ] );
-      } else if (this.taggable && this.search.length){
-        this.select(this.search)
+    typeAheadSelect () {
+      if (!this.taggable && this.filteredOptions[this.typeAheadPointer]) {
+        this.select(this.filteredOptions[this.typeAheadPointer]);
+      } else if (this.taggable && this.search.length) {
+        this.select(this.createOption(this.search));
       }
 
-      if( this.clearSearchOnSelect ) {
-        this.search = "";
+      if (this.clearSearchOnSelect) {
+        this.search = '';
       }
     },
   }

src/utility/sortAndStringify.js

@@ -0,0 +1,15 @@
+/**
+ * @param sortable {object}
+ * @return {string}
+ */
+function sortAndStringify(sortable) {
+  const ordered = {};
+
+  Object.keys(sortable).sort().forEach(key => {
+    ordered[key] = sortable[key];
+  });
+
+  return JSON.stringify(ordered);
+}
+
+export default sortAndStringify;

tests/unit/Labels.spec.js

@@ -41,4 +41,41 @@ describe("Labels", () => {
     Select.vm.$data._value = "one";
     expect(Select.vm.searchPlaceholder).not.toBeDefined();
   });
+
+  describe('getOptionLabel', () => {
+    it('will return undefined if the option lacks the label key', () => {
+      const getOptionLabel = VueSelect.props.getOptionLabel.default.bind({ label: 'label' });
+      expect(getOptionLabel({name: 'vue'})).toEqual(undefined);
+    });
+
+    it('will return a string value for a valid key', () => {
+      const getOptionLabel = VueSelect.props.getOptionLabel.default.bind({ label: 'label' });
+      expect(getOptionLabel({label: 'vue'})).toEqual('vue');
+    });
+
+    /**
+     * this test fails because of a bug where Vue executes the default contents
+     * of a slot, even if it is implemented by the consumer.
+     * @see https://github.com/vuejs/vue/issues/10224
+     * @see https://github.com/vuejs/vue/pull/10229
+     */
+    xit('will not call getOptionLabel if both scoped option slots are used and a filter is provided', () => {
+      const spy = spyOn(VueSelect.props.getOptionLabel, 'default');
+      const Select = shallowMount(VueSelect, {
+        propsData: {
+          options: [{name: 'one'}],
+          filter: () => {},
+        },
+        scopedSlots: {
+          'option': '<span class="option">{{ props.name }}</span>',
+          'selected-option': '<span class="selected">{{ props.name }}</span>',
+        },
+      });
+
+      Select.vm.select({name: 'one'});
+
+      expect(spy).toHaveBeenCalledTimes(0);
+      expect(Select.find('.selected').exists()).toBeTruthy();
+    });
+  });
 });

tests/unit/OptionComparator.spec.js

@@ -0,0 +1,31 @@
+import Select from '../../src/components/Select';
+
+describe('Comparing Options', () => {
+
+  const comparator = Select.methods.optionComparator.bind({
+    getOptionKey: Select.props.getOptionKey.default,
+  });
+
+  it('can compare numbers', () => {
+    expect(comparator(1, 2)).toBeFalsy();
+    expect(comparator(1, 1)).toBeTruthy();
+  });
+
+  it('can compare strings', () => {
+    expect(comparator('one', 'one')).toBeTruthy();
+    expect(comparator('one', 'two')).toBeFalsy();
+  });
+
+  it('can compare objects', () => {
+    //  compare ID keys
+    expect(comparator({label: 'halo', id: 1}, {label: 'halo', id: 2}))
+      .toBeFalsy();
+    //  compare objects
+    expect(comparator({label: 'halo', value: 1}, {label: 'halo', value: 1}))
+      .toBeTruthy();
+    //  compare objects with different orders
+    expect(comparator({value: 1, label: 'halo'}, {label: 'halo', value: 1}))
+      .toBeTruthy();
+  });
+
+});

tests/unit/OptionKey.spec.js

@@ -5,11 +5,11 @@ describe('Serializing Option Keys', () => {
   const getOptionKey = Select.props.getOptionKey.default;
 
   it('can serialize strings to a key', () => {
-    expect(getOptionKey('vue')).toBe('"vue"');
+    expect(getOptionKey('vue')).toBe('vue');
   });
 
   it('can serialize integers to a key', () => {
-    expect(getOptionKey(1)).toBe('1');
+    expect(getOptionKey(1)).toBe(1);
   });
 
   it('can serialize objects to a key', () => {

tests/unit/Selecting.spec.js

@@ -1,5 +1,6 @@
 import { mount, shallowMount } from "@vue/test-utils";
 import VueSelect from "../../src/components/Select.vue";
+import { mountDefault } from '../helpers';
 
 describe("VS - Selecting Values", () => {
   let defaultProps;
@@ -192,10 +193,20 @@ describe("VS - Selecting Values", () => {
         value: [{ label: "foo", value: "bar" }]
       }
     });
-    expect(Select.vm.isOptionSelected("foo")).toEqual(true);
+    expect(Select.vm.isOptionSelected({ label: "foo", value: "bar" })).toEqual(true);
   });
 
-  describe("change Event", () => {
+  it('can select two options with the same label', () => {
+    const options = [{label: 'one', id: 1}, {label: 'one', id: 2}];
+    const Select = mountDefault({options, multiple: true});
+
+    Select.vm.select({label: 'one', id: 1});
+    Select.vm.select({label: 'one', id: 2});
+
+    expect(Select.vm.selectedValue).toEqual(options);
+  });
+
+  describe("input Event", () => {
     it("will trigger the input event when the selection changes", () => {
       const Select = shallowMount(VueSelect);
       Select.vm.select("bar");

tests/unit/Tagging.spec.js

@@ -1,6 +1,8 @@
 import { searchSubmit, selectWithProps } from "../helpers";
+import Select from '../../src/components/Select';
 
 describe("When Tagging Is Enabled", () => {
+
   it("can determine if a given option string already exists", () => {
     const Select = selectWithProps({ taggable: true, options: ["one", "two"] });
     expect(Select.vm.optionExists("one")).toEqual(true);
@@ -13,8 +15,8 @@ describe("When Tagging Is Enabled", () => {
       options: [{ label: "one" }, { label: "two" }]
     });
 
-    expect(Select.vm.optionExists("one")).toEqual(true);
-    expect(Select.vm.optionExists("three")).toEqual(false);
+    expect(Select.vm.optionExists({label: "one"})).toEqual(true);
+    expect(Select.vm.optionExists({label: "three"})).toEqual(false);
   });
 
   it("can determine if a given option object already exists when using custom labels", () => {
@@ -24,8 +26,10 @@ describe("When Tagging Is Enabled", () => {
       label: "foo"
     });
 
-    expect(Select.vm.optionExists("one")).toEqual(true);
-    expect(Select.vm.optionExists("three")).toEqual(false);
+    const createOption = (text) => Select.vm.createOption(text);
+
+    expect(Select.vm.optionExists(createOption("one"))).toEqual(true);
+    expect(Select.vm.optionExists(createOption("three"))).toEqual(false);
   });
 
   it("can add the current search text as the first item in the options list", () => {
@@ -228,9 +232,11 @@ describe("When Tagging Is Enabled", () => {
       multiple: true,
       options: [{ label: "two" }]
     });
+    const spy = jest.spyOn(Select.vm, 'select');
 
     searchSubmit(Select, "one");
     expect(Select.vm.selectedValue).toEqual([{ label: "one" }]);
+    expect(spy).lastCalledWith({label: 'one'});
     expect(Select.vm.search).toEqual("");
 
     searchSubmit(Select, "one");

tests/unit/utility/sortAndStringify.spec.js

@@ -0,0 +1,14 @@
+import sortAndStringify from '../../../src/utility/sortAndStringify';
+
+test('it will stringify an object', () => {
+  expect(sortAndStringify({hello: 'world'})).toEqual('{"hello":"world"}');
+});
+
+test('it will sort attributes alphabetically', () => {
+  expect(sortAndStringify({b: 'b', a: 'a'})).toEqual('{"a":"a","b":"b"}');
+});
+
+test('comparing two objects with unsorted keys', () => {
+  expect(sortAndStringify({b: 'b', a: 'a'}))
+    .toEqual(sortAndStringify({a: 'a', b: 'b'}))
+});