Call onSelect when a DataSource item changes

Summary:
Currently, we call onSelect in DataTable only when user changes their selection. At that moment, we pass the row data to the `onSelect` callback. However, if later the data changes, but the selection stays the same, we do not call `onSelect` again. As result, any listener to onSelect does not receive the latest data.
In this diff, we start calling `onSelect` when the selection does not change, but the underlying data does.

Reviewed By: mweststrate

Differential Revision: D37520346

fbshipit-source-id: a88d34654e9ad0721caf5918dde49b86ba20fc1f

desktop/flipper-plugin/src/data-source/DataSource.tsx

@@ -472,7 +472,7 @@ class DataSourceView<T, KeyType> {
    */
   public windowEnd = 0;
 
-  private outputChangeListener?: (change: OutputChange) => void;
+  private outputChangeListeners = new Set<(change: OutputChange) => void>();
 
   /**
    * This is the base view data, that is filtered and sorted, but not reversed or windowed
@@ -520,11 +520,11 @@ class DataSourceView<T, KeyType> {
     this.windowEnd = end;
   }
 
-  public setListener(listener?: (change: OutputChange) => void) {
+  public addListener(listener: (change: OutputChange) => void) {
-    if (this.outputChangeListener && listener) {
+    this.outputChangeListeners.add(listener);
-      console.warn('outputChangeListener already set');
+    return () => {
-    }
+      this.outputChangeListeners.delete(listener);
-    this.outputChangeListener = listener;
+    };
   }
 
   public setSortBy(sortBy: undefined | keyof T | ((a: T) => Primitive)) {
@@ -617,23 +617,27 @@ class DataSourceView<T, KeyType> {
     };
   }
 
+  private notifyAllListeners(change: OutputChange) {
+    this.outputChangeListeners.forEach((listener) => listener(change));
+  }
+
   private notifyItemUpdated(viewIndex: number) {
     viewIndex = this.normalizeIndex(viewIndex);
     if (
-      !this.outputChangeListener ||
+      !this.outputChangeListeners.size ||
       viewIndex < this.windowStart ||
       viewIndex >= this.windowEnd
     ) {
       return;
     }
-    this.outputChangeListener({
+    this.notifyAllListeners({
       type: 'update',
       index: viewIndex,
     });
   }
 
   private notifyItemShift(index: number, delta: number) {
-    if (!this.outputChangeListener) {
+    if (!this.outputChangeListeners.size) {
       return;
     }
     let viewIndex = this.normalizeIndex(index);
@@ -641,7 +645,7 @@ class DataSourceView<T, KeyType> {
       viewIndex -= delta; // we need to correct for normalize already using the new length after applying this change
     }
     // Idea: we could add an option to automatically shift the window for before events.
-    this.outputChangeListener({
+    this.notifyAllListeners({
       type: 'shift',
       delta,
       index: viewIndex,
@@ -656,7 +660,7 @@ class DataSourceView<T, KeyType> {
   }
 
   private notifyReset(count: number) {
-    this.outputChangeListener?.({
+    this.notifyAllListeners({
       type: 'reset',
       newCount: count,
     });

desktop/flipper-plugin/src/data-source/DataSourceRendererStatic.tsx

@@ -66,7 +66,7 @@ export const DataSourceRendererStatic: <T extends object, C>(
       let unmounted = false;
 
       dataSource.view.setWindow(0, dataSource.limit);
-      dataSource.view.setListener((_event) => {
+      const unsubscribe = dataSource.view.addListener((_event) => {
         if (unmounted) {
           return;
         }
@@ -75,7 +75,7 @@ export const DataSourceRendererStatic: <T extends object, C>(
 
       return () => {
         unmounted = true;
-        dataSource.view.setListener(undefined);
+        unsubscribe();
       };
     },
     [dataSource, setForceUpdate, useFixedRowHeight],

desktop/flipper-plugin/src/data-source/DataSourceRendererVirtual.tsx

@@ -170,7 +170,7 @@ export const DataSourceRendererVirtual: <T extends object, C>(
         }
       }
 
-      dataSource.view.setListener((event) => {
+      const unsubscribe = dataSource.view.addListener((event) => {
         switch (event.type) {
           case 'reset':
             rerender(UpdatePrio.HIGH, true);
@@ -201,7 +201,7 @@ export const DataSourceRendererVirtual: <T extends object, C>(
 
       return () => {
         unmounted = true;
-        dataSource.view.setListener(undefined);
+        unsubscribe();
       };
     },
     [dataSource, setForceUpdate, useFixedRowHeight, isUnitTest],

desktop/flipper-plugin/src/data-source/__tests__/datasource-basics.node.tsx

@@ -489,9 +489,9 @@ function testEvents<T>(
 ): any[] {
   const ds = createDataSource<T, keyof T>(initial, {key});
   const events: any[] = [];
-  ds.view.setListener((e) => events.push(e));
+  const unsubscribe = ds.view.addListener((e) => events.push(e));
   op(ds);
-  ds.view.setListener(undefined);
+  unsubscribe();
   return events;
 }
 

desktop/flipper-plugin/src/ui/MasterDetail.tsx

@@ -104,7 +104,8 @@ export function MasterDetail<T extends object>({
 
   // if a tableManagerRef is provided, we piggy back on that same ref
   // eslint-disable-next-line
-  const tableManagerRef = tableProps.tableManagerRef ?? createRef<undefined | DataTableManager<T>>();
+  const tableManagerRef =
+    tableProps.tableManagerRef ?? createRef<undefined | DataTableManager<T>>();
 
   const pausedState = useValue(isPaused, false);
 

desktop/flipper-plugin/src/ui/data-table/DataTable.tsx

@@ -54,6 +54,7 @@ import {debounce} from 'lodash';
 import {useInUnitTest} from '../../utils/useInUnitTest';
 import {createDataSource} from '../../state/createDataSource';
 import {HighlightProvider} from '../Highlight';
+import {useLatestRef} from '../../utils/useLatestRef';
 
 type DataTableBaseProps<T = any> = {
   columns: DataTableColumn<T>[];
@@ -179,6 +180,26 @@ export function DataTable<T extends object>(
 
   const {columns, selection, searchValue, sorting} = tableState;
 
+  const latestSelectionRef = useLatestRef(selection);
+  const latestOnSelectRef = useLatestRef(onSelect);
+  useEffect(() => {
+    if (dataSource) {
+      const unsubscribe = dataSource.view.addListener((change) => {
+        if (
+          change.type === 'update' &&
+          latestSelectionRef.current.items.has(change.index)
+        ) {
+          latestOnSelectRef.current?.(
+            getSelectedItem(dataSource, latestSelectionRef.current),
+            getSelectedItems(dataSource, latestSelectionRef.current),
+          );
+        }
+      });
+
+      return unsubscribe;
+    }
+  }, [dataSource, latestSelectionRef, latestOnSelectRef]);
+
   const visibleColumns = useMemo(
     () => columns.filter((column) => column.visible),
     [columns],

desktop/flipper-plugin/src/ui/data-table/__tests__/DataTable.node.tsx

@@ -737,6 +737,8 @@ test('selection always has the latest state', () => {
   act(() => {
     ds.update(2, item3updated);
   });
+  expect(events.splice(0)).toEqual([[item3updated, [item3updated]]]);
+
   act(() => {
     ref.current!.addRangeToSelection(0, 0);
   });
@@ -745,5 +747,16 @@ test('selection always has the latest state', () => {
     [item1, [item1, item3updated]], // update reflected in callback!
   ]);
 
+  const item1updated = {
+    title: 'item 1 updated',
+    done: false,
+  };
+  act(() => {
+    ds.update(0, item1updated);
+  });
+  expect(events.splice(0)).toEqual([
+    [item1updated, [item1updated, item3updated]], // update reflected in callback!
+  ]);
+
   rendering.unmount();
 });