Implemented remove operation

Summary: Implemented `remove`, which, for a typical data source should not be needed. But that would be famous last words and wanted to prevent painting ourselves in a corner, so implemented it. Also because part of the logic is need for the `shift` operation (see next diff), which is much more important.

Reviewed By: priteshrnandgaonkar

Differential Revision: D26883672

fbshipit-source-id: 0dbfcdd3d5a16c4a2d53b0272000d183c67d0034

desktop/flipper-plugin/src/state/datasource/DataSource.tsx

@@ -38,8 +38,13 @@ type UpdateEvent<T> = {
   oldVisible: boolean;
   index: number;
 };
+type RemoveEvent<T> = {
+  type: 'remove';
+  entry: Entry<T>;
+  index: number;
+};
 
-type DataEvent<T> = AppendEvent<T> | UpdateEvent<T>;
+type DataEvent<T> = AppendEvent<T> | UpdateEvent<T> | RemoveEvent<T>;
 
 type Entry<T> = {
   value: T;
@@ -259,6 +264,48 @@ export class DataSource<
     });
   }
 
+  /**
+   * @param index
+   *
+   * Warning: this operation can be O(n) if a key is set
+   */
+  remove(index: number) {
+    if (index < 0 || index >= this._records.length) {
+      throw new Error('Out of bounds: ' + index);
+    }
+    const entry = this._records.splice(index, 1)[0];
+    if (this.keyAttribute) {
+      const key = this.getKey(entry.value);
+      this._recordsById.delete(key);
+      this.idToIndex.delete(key);
+      // Optimization: this is O(n)! Should be done as an async job
+      this.idToIndex.forEach((keyIndex, key) => {
+        if (keyIndex > index) this.idToIndex.set(key, keyIndex - 1);
+      });
+    }
+    this.emitDataEvent({
+      type: 'remove',
+      index,
+      entry,
+    });
+  }
+
+  /**
+   * Removes the item with the given key from this dataSource.
+   * Returns false if no record with the given key was found
+   *
+   * Warning: this operation can be O(n) if a key is set
+   */
+  removeByKey(keyValue: KEY_TYPE): boolean {
+    this.assertKeySet();
+    const index = this.idToIndex.get(keyValue);
+    if (index === undefined) {
+      return false;
+    }
+    this.remove(index);
+    return true;
+  }
+
   /**
    * Removes the first N entries.
    * @param amount
@@ -347,13 +394,13 @@ export class DataSource<
     );
   }
 
-  emitDataEvent(event: DataEvent<T>) {
+  private emitDataEvent(event: DataEvent<T>) {
     this.dataUpdateQueue.push(event);
     // TODO: schedule
     this.processEvents();
   }
 
-  normalizeIndex(viewIndex: number): number {
+  private normalizeIndex(viewIndex: number): number {
     return this.reverse ? this.output.length - 1 - viewIndex : viewIndex;
   }
 
@@ -365,7 +412,7 @@ export class DataSource<
     return this.output[this.normalizeIndex(viewIndex)];
   }
 
-  notifyItemUpdated(viewIndex: number) {
+  private notifyItemUpdated(viewIndex: number) {
     viewIndex = this.normalizeIndex(viewIndex);
     if (
       !this.outputChangeListener ||
@@ -380,7 +427,7 @@ export class DataSource<
     });
   }
 
-  notifyItemShift(index: number, delta: number) {
+  private notifyItemShift(index: number, delta: number) {
     if (!this.outputChangeListener) {
       return;
     }
@@ -403,19 +450,19 @@ export class DataSource<
     });
   }
 
-  notifyReset(count: number) {
+  private notifyReset(count: number) {
     this.outputChangeListener?.({
       type: 'reset',
       newCount: count,
     });
   }
 
-  processEvents() {
+  private processEvents() {
     const events = this.dataUpdateQueue.splice(0);
     events.forEach(this.processEvent);
   }
 
-  processEvent = (event: DataEvent<T>) => {
+  private processEvent = (event: DataEvent<T>) => {
     const {entry} = event;
     const {output, sortBy, filter} = this;
     switch (event.type) {
@@ -475,12 +522,29 @@ export class DataSource<
         }
         break;
       }
+      case 'remove': {
+        // filter active, and not visible? short circuilt
+        if (!entry.visible) {
+          return;
+        }
+        // no sorting, no filter?
+        if (!sortBy && !filter) {
+          output.splice(event.index, 1);
+          this.notifyItemShift(event.index, -1);
+        } else {
+          // sorting or filter is active, find the actual location
+          const existingIndex = this.getSortedIndex(entry, event.entry.value);
+          output.splice(existingIndex, 1);
+          this.notifyItemShift(existingIndex, -1);
+        }
+        break;
+      }
       default:
         throw new Error('unknown event type');
     }
   };
 
-  rebuildOutput() {
+  private rebuildOutput() {
     const {sortBy, filter, sortHelper} = this;
     // copy base array or run filter (with side effecty update of visible)
     // TODO: pending on the size, should we batch this in smaller steps? (and maybe merely reuse append)

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

@@ -45,6 +45,9 @@ test('can create a datasource', () => {
 
   ds.update(1, submitBug);
   expect(ds.records[1]).toBe(submitBug);
+
+  ds.remove(0);
+  expect(ds.records[0]).toBe(submitBug);
 });
 
 test('can create a keyed datasource', () => {
@@ -87,6 +90,14 @@ test('can create a keyed datasource', () => {
   ds.upsert(trash);
   expect(ds.records[2]).toBe(trash);
   expect(ds.recordsById.get('trash')).toBe(trash);
+
+  // delete by key
+  expect(ds.records).toEqual([eatCookie, newBug, trash]);
+  expect(ds.removeByKey('bug')).toBe(true);
+  expect(ds.records).toEqual([eatCookie, trash]);
+  expect(ds.indexOfKey('bug')).toBe(-1);
+  expect(ds.indexOfKey('cookie')).toBe(0);
+  expect(ds.indexOfKey('trash')).toBe(1);
 });
 
 test('throws on invalid keys', () => {
@@ -99,6 +110,14 @@ test('throws on invalid keys', () => {
   }).toThrow(`Duplicate key: 'cookie'`);
 });
 
+test('removing invalid keys', () => {
+  const ds = createDataSource<Todo>([eatCookie], 'id');
+  expect(ds.removeByKey('trash')).toBe(false);
+  expect(() => {
+    ds.remove(1);
+  }).toThrowError('Out of bounds');
+});
+
 test('sorting works', () => {
   const ds = createDataSource<Todo>([eatCookie, drinkCoffee]);
   ds.setSortBy((todo) => todo.title);
@@ -168,6 +187,10 @@ test('sorting preserves insertion order with equal keys', () => {
   ds.update(4, b3r);
   expect(ds.records).toEqual([b1, c, b2r, a, b3r, b4]);
   expect(unwrap(ds.output)).toEqual([a, b3r, b1, b2r, b4, c]);
+
+  ds.remove(3);
+  expect(ds.records).toEqual([b1, c, b2r, b3r, b4]);
+  expect(unwrap(ds.output)).toEqual([b3r, b1, b2r, b4, c]);
 });
 
 test('filter + sort', () => {
@@ -209,11 +232,13 @@ test('filter + sort', () => {
   ds.update(2, submitBug);
   expect(unwrap(ds.output)).toEqual([newCoffee, b, newCookie, submitBug]);
 
+  ds.remove(3); // a
+  ds.remove(3); // b
+  expect(unwrap(ds.output)).toEqual([newCoffee, newCookie, submitBug]);
+
   ds.setFilter(undefined);
   expect(unwrap(ds.output)).toEqual([
     newCoffee,
-    a,
-    b,
     drinkCoffee,
     newCookie,
     submitBug,
@@ -225,8 +250,6 @@ test('filter + sort', () => {
     newCookie,
     drinkCoffee,
     submitBug,
-    a,
-    b,
     newCoffee,
   ]);
 });
@@ -407,6 +430,9 @@ test('reverse with sorting', () => {
   };
   ds.update(4, b3r);
   expect(ds.getOutput()).toEqual([c, b4, b2r, b1, b3r, a]);
+
+  ds.remove(4);
+  expect(ds.getOutput()).toEqual([c, b4, b2r, b1, a]);
 });
 
 test('reset', () => {
@@ -443,8 +469,9 @@ test('clear', () => {
 function testEvents<T>(
   initial: T[],
   op: (ds: DataSource<T, any, any>) => void,
+  key?: keyof T,
 ): any[] {
-  const ds = createDataSource<T>(initial);
+  const ds = createDataSource<T>(initial, key);
   const events: any[] = [];
   ds.setOutputChangeListener((e) => events.push(e));
   op(ds);
@@ -543,18 +570,22 @@ test('it emits the right events - reversed view change with filter', () => {
       ds.setReversed(true);
       ds.setFilter((x) => ['a', 'b'].includes(x));
       // [b, a]
-      ds.update(0, 'x');
+      ds.update(0, 'x'); // x b
       // [b, ]
       expect(ds.getItem(0)).toEqual('b');
       expect(ds.output.length).toBe(1);
-      ds.append('y');
+      ds.append('y'); // x b y
       // [b, ]
-      ds.append('c');
+      ds.append('c'); // x b y c
       // [b, ]
-      ds.append('a');
+      ds.append('a'); // x b y c a
       // [b, a]
-      ds.append('a');
+      ds.append('a'); // x b y c a a
       // [b, a, a] // N.b. the new a is in the *middle*
+      ds.remove(2); // x b c a a
+      // no effect
+      ds.remove(4); // this removes the second a in input, so the first a in the outpat!
+      // [b, a]
     }),
   ).toEqual([
     {newCount: 2, type: 'reset'},
@@ -563,5 +594,47 @@ test('it emits the right events - reversed view change with filter', () => {
     {index: 1, delta: -1, location: 'in', newCount: 1, type: 'shift'}, // remove a
     {index: 1, delta: 1, location: 'in', newCount: 2, type: 'shift'},
     {index: 1, delta: 1, location: 'in', newCount: 3, type: 'shift'},
+    {index: 1, delta: -1, location: 'in', newCount: 2, type: 'shift'},
+  ]);
+});
+
+test('basic remove', () => {
+  const completedBug = {id: 'bug', title: 'fixed bug', done: true};
+  expect(
+    testEvents(
+      [drinkCoffee, eatCookie, submitBug],
+      (ds) => {
+        ds.setWindow(0, 100);
+        ds.remove(0);
+        expect(ds.getOutput()).toEqual([eatCookie, submitBug]);
+        expect(ds.recordsById.get('bug')).toBe(submitBug);
+        expect(ds.recordsById.get('coffee')).toBeUndefined();
+        expect(ds.recordsById.get('cookie')).toBe(eatCookie);
+        ds.upsert(completedBug);
+        ds.removeByKey('cookie');
+        expect(ds.getOutput()).toEqual([completedBug]);
+        expect(ds.recordsById.get('bug')).toBe(completedBug);
+      },
+      'id',
+    ),
+  ).toEqual([
+    {
+      type: 'shift',
+      newCount: 2,
+      location: 'in',
+      index: 0,
+      delta: -1,
+    },
+    {
+      type: 'update',
+      index: 1,
+    },
+    {
+      type: 'shift',
+      index: 0,
+      location: 'in',
+      newCount: 1,
+      delta: -1,
+    },
   ]);
 });

desktop/flipper-plugin/tsconfig.json

@@ -14,6 +14,5 @@
   ],
   "exclude": [
     "node_modules",
-    "**/__tests__/*"
   ]
 }

desktop/yarn.lock

@@ -2252,7 +2252,15 @@
   dependencies:
     "@types/istanbul-lib-report" "*"
 
-"@types/jest@26", "@types/jest@26.x", "@types/jest@^26", "@types/jest@^26.0.20":
+"@types/jest@26", "@types/jest@26.x", "@types/jest@^26":
+  version "26.0.15"
+  resolved "https://registry.yarnpkg.com/@types/jest/-/jest-26.0.15.tgz#12e02c0372ad0548e07b9f4e19132b834cb1effe"
+  integrity sha512-s2VMReFXRg9XXxV+CW9e5Nz8fH2K1aEhwgjUqPPbQd7g95T0laAcvLv032EhFHIa5GHsZ8W7iJEQVaJq6k3Gog==
+  dependencies:
+    jest-diff "^26.0.0"
+    pretty-format "^26.0.0"
+
+"@types/jest@^26.0.20":
   version "26.0.20"
   resolved "https://registry.yarnpkg.com/@types/jest/-/jest-26.0.20.tgz#cd2f2702ecf69e86b586e1f5223a60e454056307"
   integrity sha512-9zi2Y+5USJRxd0FsahERhBwlcvFh6D2GLQnY2FH2BzK8J9s9omvNHIbvABwIluXa0fD8XVKMLTO0aOEuUfACAA==