[select] Add optional "itemsEqual" comparator prop to QueryList and related components

packages/select/src/common/itemListRenderer.ts

@@ -9,7 +9,10 @@
  * An `itemListRenderer` receives this object as its sole argument.
  */
 export interface IItemListRendererProps<T> {
-    /** The currently focused item (for keyboard interactions). */
+    /**
+     * The currently focused item (for keyboard interactions), or `null` to
+     * indicate that no item is active.
+     */
     activeItem: T | null;
 
     /**

packages/select/src/common/listItemsProps.ts

@@ -4,16 +4,27 @@
 * Licensed under the terms of the LICENSE file distributed with this project.
 */
 
-import { IProps } from "@blueprintjs/core";
+import { IProps, Utils } from "@blueprintjs/core";
 import { ItemListRenderer } from "./itemListRenderer";
 import { ItemRenderer } from "./itemRenderer";
 import { ItemListPredicate, ItemPredicate } from "./predicate";
 
+/**
+ * Equality test comparator to determine if two {@link IListItemsProps} items are equivalent.
+ * @return `true` if the two items are equivalent.
+ */
+export type ItemsEqualComparator<T> = (itemA: T, itemB: T) => boolean;
+
+/**
+ * Union of all possible types for {@link IListItemsProps#itemsEqual}.
+ */
+export type ItemsEqualProp<T> = ItemsEqualComparator<T> | keyof T;
+
 /** Reusable generic props for a component that operates on a filterable, selectable list of `items`. */
 export interface IListItemsProps<T> extends IProps {
     /**
      * The currently focused item for keyboard interactions, or `null` to
-     * indicate that no item is active. If omitted, this prop will be
+     * indicate that no item is active. If omitted or `undefined`, this prop will be
      * uncontrolled (managed by the component's state). Use `onActiveItemChange`
      * to listen for updates.
      */
@@ -22,6 +33,21 @@ export interface IListItemsProps<T> extends IProps {
     /** Array of items in the list. */
     items: T[];
 
+    /**
+     * Specifies how to test if two items are equal. By default, simple strict
+     * equality (`===`) is used to compare two items.
+     *
+     * If your items have a unique identifier field, simply provide the name of
+     * a property on the item that can be compared with strict equality to
+     * determine equivalence: `itemsEqual="id"` will check `a.id === b.id`.
+     *
+     * If more complex comparison logic is required, provide an equality
+     * comparator function that returns `true` if the two items are equal. The
+     * arguments to this function will never be `null` or `undefined`, as those
+     * values are handled before calling the function.
+     */
+    itemsEqual?: ItemsEqualProp<T>;
+
     /**
      * Determine if the given item is disabled. Provide a callback function, or
      * simply provide the name of a boolean property on the item that exposes
@@ -129,3 +155,32 @@ export interface IListItemsProps<T> extends IProps {
      */
     query?: string;
 }
+
+/**
+ * Utility function for executing the {@link IListItemsProps#itemsEqual} prop to test
+ * for equality between two items.
+ * @return `true` if the two items are equivalent according to `itemsEqualProp`.
+ */
+export function executeItemsEqual<T>(
+    itemsEqualProp: ItemsEqualProp<T> | undefined,
+    itemA: T | null | undefined,
+    itemB: T | null | undefined,
+): boolean {
+    // Use strict equality if:
+    // A) Default equality check is necessary because itemsEqualProp is undefined.
+    // OR
+    // B) Either item is null/undefined. Note that null represents "no item", while
+    //    undefined represents an uncontrolled prop. This strict equality check ensures
+    //    nothing will ever be considered equivalent to an uncontrolled prop.
+    if (itemsEqualProp === undefined || itemA == null || itemB == null) {
+        return itemA === itemB;
+    }
+
+    if (Utils.isFunction(itemsEqualProp)) {
+        // itemsEqualProp is an equality comparator function, so use it
+        return itemsEqualProp(itemA, itemB);
+    } else {
+        // itemsEqualProp is a property name, so strictly compare the values of the property.
+        return itemA[itemsEqualProp] === itemB[itemsEqualProp];
+    }
+}

packages/select/src/components/omnibar/omnibar.tsx

@@ -71,7 +71,6 @@ export class Omnibar<T> extends React.PureComponent<IOmnibarProps<T>> {
             <this.TypedQueryList
                 {...restProps}
                 initialContent={initialContent}
-                onItemSelect={this.props.onItemSelect}
                 ref={this.refHandlers.queryList}
                 renderer={this.renderQueryList}
             />

packages/select/src/components/query-list/queryList.tsx

@@ -7,7 +7,13 @@
 import * as React from "react";
 
 import { DISPLAYNAME_PREFIX, IProps, Keys, Menu, Utils } from "@blueprintjs/core";
-import { IItemListRendererProps, IItemModifiers, IListItemsProps, renderFilteredItems } from "../../common";
+import {
+    executeItemsEqual,
+    IItemListRendererProps,
+    IItemModifiers,
+    IListItemsProps,
+    renderFilteredItems,
+} from "../../common";
 
 export interface IQueryListProps<T> extends IListItemsProps<T> {
     /**
@@ -167,7 +173,11 @@ export class QueryList<T> extends React.Component<IQueryListProps<T>, IQueryList
 
     public scrollActiveItemIntoView() {
         const scrollToActiveItem = this.props.scrollToActiveItem !== false;
-        const externalChangeToActiveItem = this.expectedNextActiveItem !== this.props.activeItem;
+        const externalChangeToActiveItem = !executeItemsEqual(
+            this.props.itemsEqual,
+            this.expectedNextActiveItem,
+            this.props.activeItem,
+        );
         this.expectedNextActiveItem = null;
 
         if (!scrollToActiveItem && externalChangeToActiveItem) {
@@ -214,7 +224,9 @@ export class QueryList<T> extends React.Component<IQueryListProps<T>, IQueryList
         const shouldUpdateActiveItem =
             resetActiveItem ||
             activeIndex < 0 ||
-            isItemDisabled(this.state.activeItem, activeIndex, props.itemDisabled);
+            // non-null assertion is safe because activeItem exists and was found in filteredItems
+            // (guaranteed because activeIndex >=0 here)
+            isItemDisabled(this.state.activeItem!, activeIndex, props.itemDisabled);
 
         if (hasQueryChanged && shouldUpdateActiveItem) {
             this.setActiveItem(getFirstEnabledItem(filteredItems, props.itemDisabled));
@@ -233,7 +245,7 @@ export class QueryList<T> extends React.Component<IQueryListProps<T>, IQueryList
         const { activeItem, query } = this.state;
         const matchesPredicate = this.state.filteredItems.indexOf(item) >= 0;
         const modifiers: IItemModifiers = {
-            active: activeItem === item,
+            active: executeItemsEqual(this.props.itemsEqual, activeItem, item),
             disabled: isItemDisabled(item, index, this.props.itemDisabled),
             matchesPredicate,
         };
@@ -255,7 +267,12 @@ export class QueryList<T> extends React.Component<IQueryListProps<T>, IQueryList
     private getActiveIndex(items = this.state.filteredItems) {
         const { activeItem } = this.state;
         // NOTE: this operation is O(n) so it should be avoided in render(). safe for events though.
-        return activeItem == null ? -1 : items.indexOf(activeItem);
+        for (let i = 0; i < items.length; ++i) {
+            if (executeItemsEqual(this.props.itemsEqual, items[i], activeItem)) {
+                return i;
+            }
+        }
+        return -1;
     }
 
     private getItemsParentPadding() {
@@ -308,7 +325,7 @@ export class QueryList<T> extends React.Component<IQueryListProps<T>, IQueryList
 
     /**
      * Get the next enabled item, moving in the given direction from the start
-     * index. An `undefined` return value means no suitable item was found.
+     * index. A `null` return value means no suitable item was found.
      * @param direction amount to move in each iteration, typically +/-1
      */
     private getNextActiveItem(direction: number, startIndex = this.getActiveIndex()): T | null {
@@ -361,7 +378,7 @@ function isItemDisabled<T>(item: T | null, index: number, itemDisabled?: IListIt
 
 /**
  * Get the next enabled item, moving in the given direction from the start
- * index. An `undefined` return value means no suitable item was found.
+ * index. A `null` return value means no suitable item was found.
  * @param items the list of items
  * @param isItemDisabled callback to determine if a given item is disabled
  * @param direction amount to move in each iteration, typically +/-1

packages/select/src/components/select/suggest.tsx

@@ -49,7 +49,7 @@ export interface ISuggestProps<T> extends IListItemsProps<T> {
 
     /**
      * The currently selected item, or `null` to indicate that no item is selected.
-     * If omitted, this prop will be uncontrolled (managed by the component's state).
+     * If omitted or `undefined`, this prop will be uncontrolled (managed by the component's state).
      * Use `onItemSelect` to listen for updates.
      */
     selectedItem?: T | null;

packages/select/test/index.ts

@@ -4,6 +4,7 @@
 
 import "@blueprintjs/test-commons/bootstrap";
 
+import "./listItemsPropsTests";
 import "./multiSelectTests";
 import "./omnibarTests";
 import "./queryListTests";

packages/select/test/listItemsPropsTests.ts

@@ -0,0 +1,142 @@
+/*
+ * Copyright 2019 Palantir Technologies, Inc. All rights reserved.
+ *
+ * Licensed under the terms of the LICENSE file distributed with this project.
+ */
+
+import { assert } from "chai";
+import * as sinon from "sinon";
+import { executeItemsEqual } from "../src/common/listItemsProps";
+
+
+describe("IListItemsProps Utils", () => {
+    describe("executeItemsEqual", () => {
+        // interface for a non-primitive item value
+        interface ItemObject {
+            id: string;
+            label: string;
+            listOfValues: number[];
+            nullField: null;
+        }
+
+        const ITEM_OBJECT_A: ItemObject = {
+            id: "A",
+            label: "Item A",
+            listOfValues: [1, 2],
+            nullField: null,
+        };
+
+        // Exactly the same contents as ITEM_OBJECT_A, but a different object
+        const ITEM_OBJECT_A_DUPLICATE: ItemObject = {
+            id: "A",
+            label: "Item A",
+            listOfValues: [1, 2],
+            nullField: null,
+        };
+
+        const ITEM_OBJECT_A_EQUIVALENT: ItemObject = {
+            id: "A",
+            label: "Equivalent to item A based on 'id'",
+            listOfValues: [3, 4],
+            nullField: null,
+        };
+
+        const ITEM_OBJECT_B: ItemObject = {
+            id: "B",
+            label: "Item B",
+            listOfValues: [5, 6],
+            nullField: null,
+        };
+
+        describe("itemsEqual is undefined", () => {
+            it("treats null and undefined as distinctly different", () => {
+                assert.isTrue(executeItemsEqual(undefined, null, null));
+                assert.isTrue(executeItemsEqual(undefined, undefined, undefined));
+                assert.isFalse(executeItemsEqual(undefined, null, undefined));
+                assert.isFalse(executeItemsEqual(undefined, undefined, null));
+            });
+
+            it("compares primitives correctly", () => {
+                assert.isTrue(executeItemsEqual(undefined, 42, 42));
+                assert.isFalse(executeItemsEqual(undefined, 42, 1337));
+
+                assert.isTrue(executeItemsEqual(undefined, "A", "A"));
+                assert.isFalse(executeItemsEqual(undefined, "A", "B"));
+            });
+
+            it("uses strict equality", () => {
+                assert.isTrue(executeItemsEqual(undefined, ITEM_OBJECT_A, ITEM_OBJECT_A));
+                // Duplicate objects fail strict equality test
+                assert.isFalse(executeItemsEqual(undefined, ITEM_OBJECT_A, ITEM_OBJECT_A_DUPLICATE));
+            });
+        });
+
+        describe("itemsEqual is a property name", () => {
+            it("treats null and undefined as distinctly different", () => {
+                assert.isTrue(executeItemsEqual<ItemObject>("id", null, null));
+                assert.isTrue(executeItemsEqual<ItemObject>("id", undefined, undefined));
+                assert.isFalse(executeItemsEqual<ItemObject>("id", null, undefined));
+                assert.isFalse(executeItemsEqual<ItemObject>("id", undefined, null));
+            });
+
+            it("compares primitives correctly", () => {
+                assert.isTrue(executeItemsEqual("id", ITEM_OBJECT_A, ITEM_OBJECT_A_EQUIVALENT));
+                assert.isFalse(executeItemsEqual("id", ITEM_OBJECT_A, ITEM_OBJECT_B));
+            });
+
+            it("uses strict equality", () => {
+                assert.isTrue(executeItemsEqual("listOfValues", ITEM_OBJECT_A, ITEM_OBJECT_A));
+                // "listOfValues" property is an array, so strict equality fails even though the
+                // arrays contain the same values.
+                assert.isFalse(executeItemsEqual("listOfValues", ITEM_OBJECT_A, ITEM_OBJECT_A_DUPLICATE));
+            });
+
+            it("does not incorrectly compare null to a property with a null value", () => {
+                assert.isFalse(executeItemsEqual<ItemObject>("nullField", ITEM_OBJECT_A, null));
+            });
+        });
+
+        describe("itemsEqual is a function", () => {
+            // Simple equality comparator that compares IDs of ItemObjects.
+            const equalityComparator = sinon.spy((itemA: ItemObject, itemB: ItemObject): boolean => {
+                return itemA.id === itemB.id;
+            });
+
+            beforeEach(() => {
+                equalityComparator.resetHistory();
+            });
+
+            it("treats null and undefined as distinctly different", () => {
+                assert.isTrue(executeItemsEqual<ItemObject>(equalityComparator, null, null));
+                assert.isTrue(executeItemsEqual<ItemObject>(equalityComparator, undefined, undefined));
+                assert.isFalse(executeItemsEqual<ItemObject>(equalityComparator, null, undefined));
+                assert.isFalse(executeItemsEqual<ItemObject>(equalityComparator, undefined, null));
+
+                assert(!equalityComparator.called);
+            });
+
+            it("calls the function and uses its result (true)", () => {
+                assert.isTrue(
+                    executeItemsEqual<ItemObject>(equalityComparator, ITEM_OBJECT_A, ITEM_OBJECT_A_EQUIVALENT),
+                );
+                assert(equalityComparator.calledWith(ITEM_OBJECT_A, ITEM_OBJECT_A_EQUIVALENT));
+                assert(equalityComparator.returned(true));
+            });
+
+            it("calls the function and uses its result (false)", () => {
+                assert.isFalse(executeItemsEqual<ItemObject>(equalityComparator, ITEM_OBJECT_A, ITEM_OBJECT_B));
+                assert(equalityComparator.calledWith(ITEM_OBJECT_A, ITEM_OBJECT_B));
+                assert(equalityComparator.returned(false));
+            });
+
+            it("does not call the function if one param is null/undefined", () => {
+                assert.isFalse(executeItemsEqual<ItemObject>(equalityComparator, ITEM_OBJECT_A, null));
+                assert.isFalse(executeItemsEqual<ItemObject>(equalityComparator, ITEM_OBJECT_A, undefined));
+                assert.isFalse(executeItemsEqual<ItemObject>(equalityComparator, null, ITEM_OBJECT_A));
+                assert.isFalse(executeItemsEqual<ItemObject>(equalityComparator, undefined, ITEM_OBJECT_A));
+
+                assert(!equalityComparator.called);
+            });
+        });
+    });
+});