From 564d440b4ad44b89ac4d538488939397c49d9569 Mon Sep 17 00:00:00 2001 From: Michel Weststrate Date: Tue, 16 Mar 2021 14:54:53 -0700 Subject: [PATCH] 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 --- .../src/state/datasource/DataSource.tsx | 82 ++++++++++++++-- .../__tests__/datasource-basics.node.tsx | 93 +++++++++++++++++-- desktop/flipper-plugin/tsconfig.json | 1 - desktop/yarn.lock | 10 +- 4 files changed, 165 insertions(+), 21 deletions(-) diff --git a/desktop/flipper-plugin/src/state/datasource/DataSource.tsx b/desktop/flipper-plugin/src/state/datasource/DataSource.tsx index 7349d2bed..47b4b95b2 100644 --- a/desktop/flipper-plugin/src/state/datasource/DataSource.tsx +++ b/desktop/flipper-plugin/src/state/datasource/DataSource.tsx @@ -38,8 +38,13 @@ type UpdateEvent = { oldVisible: boolean; index: number; }; +type RemoveEvent = { + type: 'remove'; + entry: Entry; + index: number; +}; -type DataEvent = AppendEvent | UpdateEvent; +type DataEvent = AppendEvent | UpdateEvent | RemoveEvent; type Entry = { 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) { + private emitDataEvent(event: DataEvent) { 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) => { + private processEvent = (event: DataEvent) => { 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) diff --git a/desktop/flipper-plugin/src/state/datasource/__tests__/datasource-basics.node.tsx b/desktop/flipper-plugin/src/state/datasource/__tests__/datasource-basics.node.tsx index 42219fc44..d62c8fa5b 100644 --- a/desktop/flipper-plugin/src/state/datasource/__tests__/datasource-basics.node.tsx +++ b/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([eatCookie], 'id'); + expect(ds.removeByKey('trash')).toBe(false); + expect(() => { + ds.remove(1); + }).toThrowError('Out of bounds'); +}); + test('sorting works', () => { const ds = createDataSource([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( initial: T[], op: (ds: DataSource) => void, + key?: keyof T, ): any[] { - const ds = createDataSource(initial); + const ds = createDataSource(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, + }, ]); }); diff --git a/desktop/flipper-plugin/tsconfig.json b/desktop/flipper-plugin/tsconfig.json index e0c4d7bd8..b9af3bc6d 100644 --- a/desktop/flipper-plugin/tsconfig.json +++ b/desktop/flipper-plugin/tsconfig.json @@ -14,6 +14,5 @@ ], "exclude": [ "node_modules", - "**/__tests__/*" ] } diff --git a/desktop/yarn.lock b/desktop/yarn.lock index d586e4ee9..0130206c0 100644 --- a/desktop/yarn.lock +++ b/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==