consolidate onChange calls (#2322)

lib/Selection/Selection.js

@@ -1,7 +1,7 @@
 import React, { useState, useMemo, useEffect, useCallback, useRef } from 'react';
 import PropTypes from 'prop-types';
 import { useIntl } from 'react-intl';
-import { UseComboboxStateChangeTypes, useCombobox } from 'downshift';
+import { useCombobox } from 'downshift';
 import classNames from 'classnames';
 import isEqual from 'lodash/isEqual';
 import formField from '../FormField';
@@ -120,11 +120,9 @@ const Selection = ({
 }) => {
   const { formatMessage } = useIntl();
   const [filterValue, updateFilterValue] = useState('');
-  const [selectedItem, updateSelectedItem] = useState(value ? getSelectedObject(value, dataOptions) : null);
   const dataLength = useRef(dataOptions?.length || 0);
   const controlRef = useProvidedRefOrCreate(inputRef);
   const awaitingChange = useRef(false);
-  const chosenItem = useRef(null);
   const options = useMemo(
     () => (asyncFilter ? dataOptions :
       filterValue ? onFilter(filterValue, dataOptions) : dataOptions),
@@ -133,15 +131,6 @@ const Selection = ({
 
   const testId = useProvidedIdOrCreate(id, 'selection-');
 
-  useEffect(() => {
-    // if dataOptions populate after the initial render, update the selectedItem state
-    // if one hasn't been found yet.
-    if (dataOptions?.length !== dataLength.current && value && selectedItem === null) {
-      updateSelectedItem(getSelectedObject(value, dataOptions));
-      dataLength.current = dataOptions.length;
-    }
-  }, [dataOptions, selectedItem, value])
-
   // we need to skip over group headings since those can neither be selectable or cursored over.
   const reducedListItems = reduceOptions(options);
   const {
@@ -152,14 +141,17 @@ const Selection = ({
     getMenuProps,
     highlightedIndex,
     getItemProps,
+    selectedItem,
+    selectItem: updateSelectedItem,
   } = useCombobox({
     items: reducedListItems,
     itemToString: defaultItemToString,
-    selectedItem,
+    initialSelectedItem: value ? getSelectedObject(value, dataOptions) : null,
     onSelectedItemChange: ({ selectedItem: newSelectedItem }) => {
-      if (onChange) {
+      // if the newSelectedItem's value matches the incoming value prop, we assume that
+      // onChange isn't necessary.
+      if (onChange && newSelectedItem?.value !== value) {
         onChange(newSelectedItem.value);
-        updateSelectedItem(newSelectedItem);
       }
     },
     isItemDisabled(item) {
@@ -171,31 +163,38 @@ const Selection = ({
         case useCombobox.stateChangeTypes.InputKeyDownEnter:
         case useCombobox.stateChangeTypes.ItemClick:
           awaitingChange.current = true;
-          chosenItem.current = changes.selectedItem;
           return changes;
         default:
           return changes;
       }
     }
   });
 
+  useEffect(() => {
+    // if dataOptions populate/change after the initial render, update the selectedItem state
+    // if one hasn't been found yet.
+    if (dataOptions?.length !== dataLength.current && value && selectedItem === null) {
+      const selected = getSelectedObject(value, dataOptions);
+      updateSelectedItem(selected);
+      dataLength.current = dataOptions.length;
+    }
+  }, [dataOptions, selectedItem, value])
+
   const valueLabel = defaultItemToString(selectedItem) || placeholder || '';
   const labelId = `sl-label-${testId}`;
   const valueId = `selected-${testId}-item`;
 
   if (awaitingChange.current) {
-    if (chosenItem.current !== null && chosenItem.current?.value === value) {
+    // a user has change the value via the dropdown and we're waiting for the value
+    // to correspond with the state...
+    if (selectedItem !== null && selectedItem?.value === value) {
       awaitingChange.current = false;
-      chosenItem.current = null;
     }
   } else if (selectedItem !== null && selectedItem?.value !== value){
-    // conform to post-render value prop changes from outside of the component,
+    // conform to post-mount value prop changes from outside of the component,
     // whether the changed value is something empty like '' or null;
     const newValue = getSelectedObject(value, dataOptions) || { value }
     updateSelectedItem(newValue);
-    if (onChange) {
-      onChange(newValue.value);
-    }
   }
 
   /** renderOptions

lib/Selection/tests/Selection-test.js

@@ -577,29 +577,32 @@ describe('Selection', () => {
 
   describe('Changing the value prop outside of render', () => {
     const changeSpy = sinon.spy();
+    const harnessHandler = (fn, val) => {
+      changeSpy(val);
+      fn(val);
+    };
     beforeEach(async () => {
+      changeSpy.resetHistory();
       await mountWithContext(
         <SingleSelectionHarness
           label="test selection"
           initValue="test2"
           options={listOptions}
-          onChange={changeSpy}
+          onChange={harnessHandler}
         />
       );
     });
 
     it('renders initial value as provided', () => selection.has({ singleValue: 'Option 2' }));
 
-    describe('Reseting the value', () => {
+    describe('Resetting the value outside of the component', () => {
       beforeEach(async () => {
         await Button('reset').click();
       });
 
-      it('removes the value', () => selection.has({ singleValue: '' }))
+      it('removes the value', () => selection.has({ singleValue: '' }));
 
-      it('calls the supplied change handler', async () => {
-        await expect(changeSpy.calledOnceWith(''));
-      })
+      it('will not trigger a onChange', async () => converge(() => { if (changeSpy.called) throw new Error('expected change handler to be called once') }));
     });
   });
 
@@ -633,6 +636,14 @@ describe('Selection', () => {
       it('updates the value', () => selection.has({ singleValue: 'Option 0' }));
 
       it('calls the supplied change handler', async () => converge(() => { if (!changeSpy.calledOnceWith('test0')) throw new Error('expected change handler to be called') }));
+
+      describe('additional renders...', () => {
+        beforeEach(async () => {
+          await Button(including('update')).click();
+        });
+
+        it('will not trigger an additional onChange', async () => converge(() => { if (!changeSpy.calledOnce) throw new Error('expected change handler to be called once') }));
+      });
     });
   });
 

lib/Selection/tests/SingleSelectionHarness.js

@@ -10,11 +10,13 @@ const SingleSelectionHarness = ({
   onChange = (fn, val) => { fn(val) },
 }) => {
   const [fieldVal, setFieldVal] = useState(initValue);
-  const [options, updateOptions] = useState(optionsProp)
+  const [options, updateOptions] = useState(optionsProp);
+  const [increment, updateIncrement] = useState(0);
   return (
     <>
       <Button onClick={() => setFieldVal('')}>reset</Button>
       <Button onClick={() => setTimeout(updateOptions(delayedOptions), 100)}>fillData</Button>
+      <Button onClick={() => updateIncrement((cur) => cur + 1)}>{`update(${increment})`}</Button>;
       <Selection
         label={label}
         value={fieldVal}