Fix search results could be unexpectedly unstable, and that `fuseOpti…

…ons.sortFn` was effectively ignored Choices-js#1106

CHANGELOG.md

@@ -9,6 +9,7 @@
 * Fix mutation APIs `setChoiceByValue`/`setChoices`/`setValue` now throw an error the Choices instance was not initialized or multiple choices instances where initialized on the same element. Prevents bad internal states from triggering unexpected errors [#1129](https://github.com/Choices-js/Choices/issues/1129)
 * Fix documentation that suggests duplicateItemsAllowed works with select-multiple, when it only works for text. [#1123](https://github.com/Choices-js/Choices/issues/1123)
 * Fix quadratic algorithm complexity (aka O(N^2) ) when filtering/search choices.
+* Fix search results could be unexpectedly unstable, and that `fuseOptions.sortFn` was effectively ignored [#1106](https://github.com/Choices-js/Choices/issues/1106)
 
 ### Bug Fixes (from 11.0.0RC1)
 * Fix possible empty `aria-label` generation on remove item button

src/scripts/choices.ts

@@ -35,7 +35,7 @@ import {
   getClassNamesSelector,
   isScrolledIntoView,
   sanitise,
-  sortByScore,
+  sortByRank,
   strToEl,
 } from './lib/utils';
 import { defaultState } from './reducers';
@@ -1128,7 +1128,6 @@ class Choices implements ChoicesInterface {
     // Create a fragment to store our list items (so we don't have to update the DOM for each item)
     const { renderSelectedChoices, searchResultLimit, renderChoiceLimit } =
       this.config;
-    const filter = this._isSearching ? sortByScore : this.config.sorter;
     const groupLookup: string[] = [];
     const appendGroupInSearch =
       this.config.appendGroupInSearch && this._isSearching;
@@ -1188,9 +1187,12 @@ class Choices implements ChoicesInterface {
       });
     }
 
-    // If sorting is enabled or the user is searching, filter choices
-    if (this.config.shouldSort || this._isSearching) {
-      normalChoices.sort(filter);
+    if (this._isSearching) {
+      // sortByRank is used to ensure stable sorting, as scores are non-unique
+      // this additionally ensures fuseOptions.sortFn is not ignored
+      normalChoices.sort(sortByRank);
+    } else if (this.config.shouldSort) {
+      normalChoices.sort(this.config.sorter);
     }
 
     let choiceLimit = rendererableChoices.length;

src/scripts/components/wrapped-select.ts

@@ -73,6 +73,7 @@ export default class WrappedSelect extends WrappedElement<HTMLSelectElement> {
       id: 0,
       groupId: 0,
       score: 0,
+      rank: 0,
       value: option.value,
       label: option.innerHTML,
       element: option,

src/scripts/interfaces/choice-full.ts

@@ -19,4 +19,5 @@ export interface ChoiceFull {
   selected: boolean;
   value: string;
   score: number;
+  rank: number;
 }

src/scripts/interfaces/search.ts

@@ -3,6 +3,7 @@ import { Options } from './options';
 export interface SearchResult<T extends object> {
   item: T;
   score: number;
+  rank: number;
 }
 
 export type SearchHandler = <T extends object>(

src/scripts/lib/choice-input.ts

@@ -65,6 +65,7 @@ export const mapInputToChoice = <T extends string | InputChoice | InputGroup>(
     id: 0, // actual ID will be assigned during _addChoice
     groupId: 0, // actual ID will be assigned during _addGroup but before _addChoice
     score: 0, // used in search
+    rank: 0, // used in search, stable sort order
     value: choice.value,
     label: choice.label || choice.value,
     active: coerceBool(choice.active),

src/scripts/lib/utils.ts

@@ -166,6 +166,13 @@ export const sortByScore = (
   return a.score - b.score;
 };
 
+export const sortByRank = (
+  a: Pick<ChoiceFull, 'rank'>,
+  b: Pick<ChoiceFull, 'rank'>,
+): number => {
+  return a.rank - b.rank;
+};
+
 export const dispatchEvent = (
   element: HTMLElement,
   type: EventType,

src/scripts/reducers/choices.ts

@@ -8,6 +8,7 @@ import {
 import { AddItemAction, RemoveItemAction } from '../actions/items';
 import { ChoiceFull } from '../interfaces/choice-full';
 import { ActionType } from '../interfaces';
+import { SearchResult } from '../interfaces/search';
 
 type ActionTypes =
   | AddChoiceAction
@@ -65,19 +66,21 @@ export default function choices(
       const { results } = action as FilterChoicesAction;
 
       // avoid O(n^2) algorithm complexity when searching/filtering choices
-      const scoreLookup: number[] = [];
+      const scoreLookup: SearchResult<ChoiceFull>[] = [];
       results.forEach((result) => {
-        scoreLookup[result.item.id] = result.score;
+        scoreLookup[result.item.id] = result;
       });
 
       return state.map((obj) => {
         const choice = obj;
-
-        if (choice.id in scoreLookup) {
-          choice.score = scoreLookup[choice.id];
+        const result = scoreLookup[choice.id];
+        if (result !== undefined) {
+          choice.score = result.score;
+          choice.rank = result.rank;
           choice.active = true;
         } else {
           choice.score = 0;
+          choice.rank = 0;
           choice.active = false;
         }
 

src/scripts/search/fuse.ts

@@ -29,5 +29,11 @@ export function searchByFuse<T extends object>(
 
   const results = fuse.search(needle);
 
-  return results as SearchResult<T>[];
+  return results.map((value, i): SearchResult<T> => {
+    return {
+      item: value.item,
+      score: value.score || 0,
+      rank: i, // If value.score is used for sorting, this can create non-stable sorts!
+    };
+  });
 }

src/scripts/search/prefix-filter.ts

@@ -25,6 +25,7 @@ export function searchByPrefixFilter<T extends object>(
       return {
         item: value,
         score: index,
+        rank: index,
       };
     });
 }

test/scripts/actions/choices.test.ts

@@ -13,6 +13,7 @@ describe('actions/choices', () => {
     id: 1,
     groupId: 1,
     score: 0,
+    rank: 0,
     disabled: false,
     elementId: '1',
     labelClass: stringToHtmlClass('test foo--bar'),

test/scripts/actions/items.test.ts

@@ -15,6 +15,7 @@ describe('actions/items', () => {
     id: 1,
     groupId: 1,
     score: 0,
+    rank: 0,
     customProperties: { test: true },
     placeholder: true,
   };

test/scripts/choices.test.ts

@@ -787,6 +787,7 @@ describe('choices', () => {
         value: 'Test',
         label: 'Test',
         score: 0,
+        rank: 0,
       };
 
       beforeEach(() => {
@@ -918,6 +919,7 @@ describe('choices', () => {
         value: 'Test',
         label: 'Test',
         score: 0,
+        rank: 0,
       };
 
       beforeEach(() => {
@@ -2079,6 +2081,7 @@ describe('choices', () => {
           placeholder: false,
           highlighted: false,
           score: 0,
+          rank: 0,
         },
         {
           id: 2,
@@ -2091,6 +2094,7 @@ describe('choices', () => {
           placeholder: false,
           highlighted: false,
           score: 0,
+          rank: 0,
         },
         {
           id: 3,
@@ -2103,6 +2107,7 @@ describe('choices', () => {
           placeholder: false,
           highlighted: false,
           score: 0,
+          rank: 0,
         },
       ];
 
@@ -2490,6 +2495,7 @@ describe('choices', () => {
           groupId: 3333,
           customProperties: {},
           score: 0,
+          rank: 0,
         };
 
         it('dispatches a REMOVE_ITEM action to the store', () => {

test/scripts/lib/utils.test.ts

@@ -11,6 +11,7 @@ import {
   parseCustomProperties,
   sortByAlpha,
   sortByScore,
+  sortByRank,
 } from '../../../src/scripts/lib/utils';
 import { EventType } from '../../../src';
 
@@ -140,6 +141,28 @@ describe('utils', () => {
     });
   });
 
+  describe('sortByRank', () => {
+    describe('sorting an array', () => {
+      it('sorts by rank ascending', () => {
+        const values = [
+          { rank: 10 },
+          { rank: 3001 },
+          { rank: 124 },
+          { rank: 400 },
+        ];
+
+        const output = values.sort(sortByRank);
+
+        expect(output).to.deep.equal([
+          { rank: 10 },
+          { rank: 124 },
+          { rank: 400 },
+          { rank: 3001 },
+        ]);
+      });
+    });
+  });
+
   describe('dispatchEvent', () => {
     it('dispatches custom event of given type on given element', () => {
       const fakeElement = {

test/scripts/reducers/choices.test.ts

@@ -29,6 +29,7 @@ describe('reducers/choices', () => {
           test: true,
         },
         score: 0,
+        rank: 0,
       };
 
       describe('passing expected values', () => {
@@ -79,6 +80,7 @@ describe('reducers/choices', () => {
           selected: false,
           active: false,
           score: 9999,
+          rank: 9999,
           customProperties: {},
           placeholder: false,
           highlighted: false,
@@ -93,6 +95,7 @@ describe('reducers/choices', () => {
           selected: true,
           active: false,
           score: 9999,
+          rank: 9999,
           customProperties: {},
           placeholder: false,
           highlighted: false,
@@ -104,12 +107,14 @@ describe('reducers/choices', () => {
       it('sets active flag based on whether choice is in passed results', () => {
         const id = 1;
         const score = 10;
+        const rank = 10;
         const active = true;
 
         const expectedResponse = {
           ...state[0],
           active,
           score,
+          rank,
         } as ChoiceFull;
 
         const actualResponse = choices(cloneObject(state), {
@@ -118,6 +123,7 @@ describe('reducers/choices', () => {
             {
               item: { id } as ChoiceFull,
               score,
+              rank,
             },
           ],
         }).find((choice) => choice.id === id);

test/scripts/reducers/items.test.ts

@@ -21,6 +21,7 @@ describe('reducers/items', () => {
         id: 1234,
         groupId: 1,
         score: 0,
+        rank: 0,
         customProperties: {
           property: 'value',
         },
@@ -83,6 +84,7 @@ describe('reducers/items', () => {
           id: 1,
           groupId: -1,
           score: 0,
+          rank: 0,
           value: 'Item one',
           label: 'Item one',
           active: false,
@@ -96,6 +98,7 @@ describe('reducers/items', () => {
           id: 2,
           groupId: -1,
           score: 0,
+          rank: 0,
           value: 'Item one',
           label: 'Item one',
           active: true,

test/scripts/search/index.test.ts

@@ -1,7 +1,9 @@
 import { expect } from 'chai';
+import { beforeEach } from 'vitest';
 import { searchByPrefixFilter } from '../../../src/scripts/search/prefix-filter';
 import { searchByFuse } from '../../../src/scripts/search/fuse';
 import { DEFAULT_CONFIG } from '../../../src';
+import { cloneObject } from '../../../src/scripts/lib/utils';
 
 export interface SearchableShape {
   label: string;
@@ -20,7 +22,9 @@ describe('search', () => {
 
   describe('fuse-full', () => {
     const search = searchByFuse;
-    process.env.SEARCH_FUSE = 'full';
+    beforeEach(() => {
+      process.env.SEARCH_FUSE = 'full';
+    });
     it('empty result', () => {
       const results = search<SearchableShape>(options, haystack, '');
       expect(results.length).eq(0);
@@ -37,11 +41,39 @@ describe('search', () => {
       );
       expect(results.length).eq(1);
     });
+
+    it('search order', () => {
+      const results = search<SearchableShape>(options, haystack, 'label');
+
+      expect(results.length).eq(haystack.length);
+      haystack.forEach((value, index) => {
+        expect(results[index].item.value).eq(value.value);
+      });
+    });
+
+    it('search order - custom sortFn', () => {
+      const opts = cloneObject(options);
+      opts.fuseOptions.sortFn = (a, b) => {
+        if (a.score === b.score) {
+          return a.idx < b.idx ? 1 : -1;
+        }
+
+        return a.score < b.score ? 1 : -1;
+      };
+      const results = search<SearchableShape>(opts, haystack, 'label');
+
+      expect(results.length).eq(haystack.length);
+      haystack.reverse().forEach((value, index) => {
+        expect(results[index].item.value).eq(value.value);
+      });
+    });
   });
 
   describe('fuse-basic', () => {
     const search = searchByFuse;
-    process.env.SEARCH_FUSE = 'basic';
+    beforeEach(() => {
+      process.env.SEARCH_FUSE = 'basic';
+    });
     it('empty result', () => {
       const results = search<SearchableShape>(options, haystack, '');
       expect(results.length).eq(0);
@@ -58,6 +90,14 @@ describe('search', () => {
       );
       expect(results.length).eq(1);
     });
+    it('search order', () => {
+      const results = search<SearchableShape>(options, haystack, 'label');
+
+      expect(results.length).eq(haystack.length);
+      haystack.forEach((value, index) => {
+        expect(results[index].item.value).eq(value.value);
+      });
+    });
   });
 
   describe('prefix-filter', () => {