From 4a4cc21d896244ba20f40fcfc61593fd4f241c20 Mon Sep 17 00:00:00 2001 From: Anton Kastritskiy Date: Thu, 28 Oct 2021 10:42:49 -0700 Subject: [PATCH] Refine DataSource id to use actual key type instead of a wide string type Summary: Current implementation uses type `string` as a key for indexing items stored in datasource. However, users can provide any key as an index which means that the type of index item can be anything, not only string. This diff introduces a more refined types for the key. It adds another requirement to provide a key property to a generic which is used to infer the index type. Reviewed By: mweststrate, aigoncharov Differential Revision: D31895751 fbshipit-source-id: 19ba907bd6f35df87e3fa442db5fc5cec6af174d --- .../src/data-source/DataSource.tsx | 57 ++++++++++--------- .../data-source/DataSourceRendererStatic.tsx | 6 +- .../data-source/DataSourceRendererVirtual.tsx | 6 +- .../__tests__/datasource-basics.node.tsx | 22 +++---- .../__tests__/datasource-perf.node.tsx | 2 +- .../flipper-plugin/src/data-source/index.tsx | 7 ++- .../src/state/createDataSource.tsx | 27 +++++---- desktop/flipper-plugin/src/ui/DataList.tsx | 10 ++-- .../src/ui/data-table/DataTable.tsx | 15 +++-- .../src/ui/data-table/DataTableManager.tsx | 10 ++-- .../src/ui/data-table/TableContextMenu.tsx | 2 +- .../src/utils/createTablePlugin.tsx | 6 +- desktop/plugins/public/network/types.tsx | 2 +- 13 files changed, 99 insertions(+), 73 deletions(-) diff --git a/desktop/flipper-plugin/src/data-source/DataSource.tsx b/desktop/flipper-plugin/src/data-source/DataSource.tsx index 0a8feaa4b..95c233c56 100644 --- a/desktop/flipper-plugin/src/data-source/DataSource.tsx +++ b/desktop/flipper-plugin/src/data-source/DataSource.tsx @@ -77,12 +77,14 @@ type OutputChange = newCount: number; }; -export type DataSourceOptions = { +export type DataSourceOptionKey = { /** * If a key is set, the given field of the records is assumed to be unique, * and it's value can be used to perform lookups and upserts. */ key?: K; +}; +export type DataSourceOptions = { /** * The maximum amount of records that this DataSource will store. * If the limit is exceeded, the oldest records will automatically be dropped to make place for the new ones @@ -90,16 +92,19 @@ export type DataSourceOptions = { limit?: number; }; -export function createDataSource( +export function createDataSource( initialSet: readonly T[], - options?: DataSourceOptions, -): DataSource; -export function createDataSource(initialSet?: readonly T[]): DataSource; -export function createDataSource( + options: DataSourceOptions & DataSourceOptionKey, +): DataSource; +export function createDataSource( + initialSet?: readonly T[], + options?: DataSourceOptions, +): DataSource; +export function createDataSource( initialSet: readonly T[] = [], - options?: DataSourceOptions, -): DataSource { - const ds = new DataSource(options?.key); + options?: DataSourceOptions & DataSourceOptionKey, +): DataSource { + const ds = new DataSource(options?.key); if (options?.limit !== undefined) { ds.limit = options.limit; } @@ -107,15 +112,15 @@ export function createDataSource( return ds as any; } -export class DataSource { +export class DataSource { private nextId = 0; private _records: Entry[] = []; - private _recordsById: Map = new Map(); + private _recordsById: Map = new Map(); /** * @readonly */ public keyAttribute: keyof T | undefined; - private idToIndex: Map = new Map(); + private idToIndex: Map = new Map(); // if we shift the window, we increase shiftOffset to correct idToIndex results, rather than remapping all values private shiftOffset = 0; @@ -131,11 +136,11 @@ export class DataSource { * * Additional views can created through the fork method. */ - public readonly view: DataSourceView; + public readonly view: DataSourceView; constructor(keyAttribute: keyof T | undefined) { this.keyAttribute = keyAttribute; - this.view = new DataSourceView(this); + this.view = new DataSourceView(this); } public get size() { @@ -154,22 +159,22 @@ export class DataSource { return unwrap(this._records[index]); } - public has(key: string) { + public has(key: KeyType) { this.assertKeySet(); return this._recordsById.has(key); } - public getById(key: string): T | undefined { + public getById(key: KeyType): T | undefined { this.assertKeySet(); return this._recordsById.get(key); } - public keys(): IterableIterator { + public keys(): IterableIterator { this.assertKeySet(); return this._recordsById.keys(); } - public entries(): IterableIterator<[string, T]> { + public entries(): IterableIterator<[KeyType, T]> { this.assertKeySet(); return this._recordsById.entries(); } @@ -198,7 +203,7 @@ export class DataSource { * Returns the index of a specific key in the *records* set. * Returns -1 if the record wansn't found */ - public getIndexOfKey(key: string): number { + public getIndexOfKey(key: KeyType): number { this.assertKeySet(); const stored = this.idToIndex.get(key); return stored === undefined ? -1 : stored + this.shiftOffset; @@ -328,7 +333,7 @@ export class DataSource { * * Warning: this operation can be O(n) if a key is set */ - public deleteByKey(keyValue: string): boolean { + public deleteByKey(keyValue: KeyType): boolean { this.assertKeySet(); const index = this.getIndexOfKey(keyValue); if (index === -1) { @@ -394,7 +399,7 @@ export class DataSource { * Returns a fork of this dataSource, that shares the source data with this dataSource, * but has it's own FSRW pipeline, to allow multiple views on the same data */ - public fork(): DataSourceView { + public fork(): DataSourceView { throw new Error( 'Not implemented. Please contact oncall if this feature is needed', ); @@ -408,7 +413,7 @@ export class DataSource { } } - private getKey(value: T): string; + private getKey(value: T): KeyType; private getKey(value: any): any { this.assertKeySet(); const key = value[this.keyAttribute!]; @@ -418,7 +423,7 @@ export class DataSource { throw new Error(`Invalid key value: '${key}'`); } - private storeIndexOfKey(key: string, index: number) { + private storeIndexOfKey(key: KeyType, index: number) { // de-normalize the index, so that on later look ups its corrected again this.idToIndex.set(key, index - this.shiftOffset); } @@ -452,8 +457,8 @@ function unwrap(entry: Entry): T { return entry?.value; } -class DataSourceView { - public readonly datasource: DataSource; +class DataSourceView { + public readonly datasource: DataSource; private sortBy: undefined | ((a: T) => Primitive) = undefined; private reverse: boolean = false; private filter?: (value: T) => boolean = undefined; @@ -474,7 +479,7 @@ class DataSourceView { */ private _output: Entry[] = []; - constructor(datasource: DataSource) { + constructor(datasource: DataSource) { this.datasource = datasource; } diff --git a/desktop/flipper-plugin/src/data-source/DataSourceRendererStatic.tsx b/desktop/flipper-plugin/src/data-source/DataSourceRendererStatic.tsx index 25359feb9..b2d1ddaf7 100644 --- a/desktop/flipper-plugin/src/data-source/DataSourceRendererStatic.tsx +++ b/desktop/flipper-plugin/src/data-source/DataSourceRendererStatic.tsx @@ -16,7 +16,7 @@ type DataSourceProps = { /** * The data source to render */ - dataSource: DataSource; + dataSource: DataSource; /** * additional context that will be passed verbatim to the itemRenderer, so that it can be easily memoized */ @@ -32,7 +32,9 @@ type DataSourceProps = { defaultRowHeight: number; onKeyDown?: React.KeyboardEventHandler; onUpdateAutoScroll?(autoScroll: boolean): void; - emptyRenderer?: null | ((dataSource: DataSource) => React.ReactElement); + emptyRenderer?: + | null + | ((dataSource: DataSource) => React.ReactElement); }; /** diff --git a/desktop/flipper-plugin/src/data-source/DataSourceRendererVirtual.tsx b/desktop/flipper-plugin/src/data-source/DataSourceRendererVirtual.tsx index 50707166f..e01394094 100644 --- a/desktop/flipper-plugin/src/data-source/DataSourceRendererVirtual.tsx +++ b/desktop/flipper-plugin/src/data-source/DataSourceRendererVirtual.tsx @@ -39,7 +39,7 @@ type DataSourceProps = { /** * The data source to render */ - dataSource: DataSource; + dataSource: DataSource; /** * Automatically scroll if the user is near the end? */ @@ -66,7 +66,9 @@ type DataSourceProps = { offset: number, ): void; onUpdateAutoScroll?(autoScroll: boolean): void; - emptyRenderer?: null | ((dataSource: DataSource) => React.ReactElement); + emptyRenderer?: + | null + | ((dataSource: DataSource) => React.ReactElement); }; /** diff --git a/desktop/flipper-plugin/src/data-source/__tests__/datasource-basics.node.tsx b/desktop/flipper-plugin/src/data-source/__tests__/datasource-basics.node.tsx index d0ad40e6a..9612f7f54 100644 --- a/desktop/flipper-plugin/src/data-source/__tests__/datasource-basics.node.tsx +++ b/desktop/flipper-plugin/src/data-source/__tests__/datasource-basics.node.tsx @@ -34,7 +34,7 @@ function unwrap(array: readonly {value: T}[]): readonly T[] { return array.map((entry) => entry.value); } -function rawOutput(ds: DataSource): readonly T[] { +function rawOutput(ds: DataSource): readonly T[] { // @ts-ignore const output = ds.view._output; return unwrap(output); @@ -60,7 +60,7 @@ test('can create a datasource', () => { }); test('can create a keyed datasource', () => { - const ds = createDataSource([eatCookie], {key: 'id'}); + const ds = createDataSource([eatCookie], {key: 'id'}); expect(ds.records()).toEqual([eatCookie]); ds.append(drinkCoffee); @@ -110,7 +110,7 @@ test('can create a keyed datasource', () => { }); test('throws on invalid keys', () => { - const ds = createDataSource([eatCookie], {key: 'id'}); + const ds = createDataSource([eatCookie], {key: 'id'}); expect(() => { ds.append({id: '', title: 'test'}); }).toThrow(`Invalid key value: ''`); @@ -120,7 +120,7 @@ test('throws on invalid keys', () => { }); test('throws on update causing duplicate key', () => { - const ds = createDataSource([eatCookie, submitBug], {key: 'id'}); + const ds = createDataSource([eatCookie, submitBug], {key: 'id'}); expect(() => { ds.update(0, {id: 'bug', title: 'oops'}); }).toThrow( @@ -129,7 +129,7 @@ test('throws on update causing duplicate key', () => { }); test('removing invalid keys', () => { - const ds = createDataSource([eatCookie], {key: 'id'}); + const ds = createDataSource([eatCookie], {key: 'id'}); expect(ds.deleteByKey('trash')).toBe(false); expect(() => { ds.delete(1); @@ -263,7 +263,7 @@ test('filter + sort', () => { }); test('filter + sort + index', () => { - const ds = createDataSource([eatCookie, drinkCoffee, submitBug], { + const ds = createDataSource([eatCookie, drinkCoffee, submitBug], { key: 'id', }); @@ -315,7 +315,7 @@ test('filter + sort + index', () => { }); test('filter', () => { - const ds = createDataSource([eatCookie, drinkCoffee, submitBug], { + const ds = createDataSource([eatCookie, drinkCoffee, submitBug], { key: 'id', }); @@ -448,7 +448,7 @@ test('reverse with sorting', () => { }); test('reset', () => { - const ds = createDataSource([submitBug, drinkCoffee, eatCookie], { + const ds = createDataSource([submitBug, drinkCoffee, eatCookie], { key: 'id', }); ds.view.setSortBy('title'); @@ -462,7 +462,7 @@ test('reset', () => { }); test('clear', () => { - const ds = createDataSource([submitBug, drinkCoffee, eatCookie], { + const ds = createDataSource([submitBug, drinkCoffee, eatCookie], { key: 'id', }); ds.view.setSortBy('title'); @@ -484,10 +484,10 @@ test('clear', () => { function testEvents( initial: T[], - op: (ds: DataSource) => void, + op: (ds: DataSource) => void, key?: keyof T, ): any[] { - const ds = createDataSource(initial, {key}); + const ds = createDataSource(initial, {key}); const events: any[] = []; ds.view.setListener((e) => events.push(e)); op(ds); diff --git a/desktop/flipper-plugin/src/data-source/__tests__/datasource-perf.node.tsx b/desktop/flipper-plugin/src/data-source/__tests__/datasource-perf.node.tsx index 474d13042..6434e83e4 100644 --- a/desktop/flipper-plugin/src/data-source/__tests__/datasource-perf.node.tsx +++ b/desktop/flipper-plugin/src/data-source/__tests__/datasource-perf.node.tsx @@ -32,7 +32,7 @@ function generateTodos(amount: number): Todo[] { const defaultFilter = (t: Todo) => !t.done; -type DataSourceish = DataSource | FakeDataSource; +type DataSourceish = DataSource | FakeDataSource; // NOTE: this run in jest, which is not optimal for perf, but should give some idea // make sure to use the `yarn watch` script in desktop root, so that the garbage collector is exposed diff --git a/desktop/flipper-plugin/src/data-source/index.tsx b/desktop/flipper-plugin/src/data-source/index.tsx index 83c38e45e..128844105 100644 --- a/desktop/flipper-plugin/src/data-source/index.tsx +++ b/desktop/flipper-plugin/src/data-source/index.tsx @@ -7,7 +7,12 @@ * @format */ -export {DataSource, createDataSource, DataSourceOptions} from './DataSource'; +export { + DataSource, + createDataSource, + DataSourceOptions, + DataSourceOptionKey, +} from './DataSource'; export { DataSourceRendererVirtual, DataSourceVirtualizer, diff --git a/desktop/flipper-plugin/src/state/createDataSource.tsx b/desktop/flipper-plugin/src/state/createDataSource.tsx index a820c6c7a..c3c662256 100644 --- a/desktop/flipper-plugin/src/state/createDataSource.tsx +++ b/desktop/flipper-plugin/src/state/createDataSource.tsx @@ -11,13 +11,11 @@ import { DataSource, createDataSource as baseCreateDataSource, DataSourceOptions as BaseDataSourceOptions, + DataSourceOptionKey as BaseDataSourceOptionKey, } from '../data-source/index'; import {registerStorageAtom} from '../plugin/PluginBase'; -type CreateDataSourceOptions = BaseDataSourceOptions< - T, - K -> & { +type DataSourceOptions = BaseDataSourceOptions & { /** * Should this state persist when exporting a plugin? * If set, the dataSource will be saved / loaded under the key provided @@ -25,16 +23,21 @@ type CreateDataSourceOptions = BaseDataSourceOptions< persist?: string; }; -export function createDataSource( +export function createDataSource( initialSet: readonly T[], - options: CreateDataSourceOptions, -): DataSource; -export function createDataSource(initialSet?: readonly T[]): DataSource; -export function createDataSource( + options: DataSourceOptions & BaseDataSourceOptionKey, +): DataSource; +export function createDataSource( + initialSet?: readonly T[], + options?: DataSourceOptions, +): DataSource; +export function createDataSource( initialSet: readonly T[] = [], - options?: CreateDataSourceOptions, -): DataSource { - const ds = baseCreateDataSource(initialSet, options); + options?: DataSourceOptions & BaseDataSourceOptionKey, +): DataSource { + const ds = options + ? baseCreateDataSource(initialSet, options) + : baseCreateDataSource(initialSet); registerStorageAtom(options?.persist, ds); return ds; } diff --git a/desktop/flipper-plugin/src/ui/DataList.tsx b/desktop/flipper-plugin/src/ui/DataList.tsx index 3bdd2cda4..bd3e094c1 100644 --- a/desktop/flipper-plugin/src/ui/DataList.tsx +++ b/desktop/flipper-plugin/src/ui/DataList.tsx @@ -47,7 +47,7 @@ type DataListBaseProps = { /** * Items to display. Per item at least a title and unique id should be provided */ - items: DataSource | readonly Item[]; + items: DataSource | readonly Item[]; /** * Custom render function. By default the component will render the `title` in bold and description (if any) below it */ @@ -75,10 +75,12 @@ export type DataListProps = DataListBaseProps & // Some table props are set by DataList instead, so override them Omit, 'records' | 'dataSource' | 'columns' | 'onSelect'>; -export const DataList: ((props: DataListProps) => React.ReactElement) & { +export const DataList: (( + props: DataListProps, +) => React.ReactElement) & { Item: React.FC; } = Object.assign( - function ({ + function ({ onSelect: baseOnSelect, selection, className, @@ -151,7 +153,7 @@ export const DataList: ((props: DataListProps) => React.ReactElement) & { return ( - + {...tableProps} tableManagerRef={tableManagerRef} records={Array.isArray(items) ? items : undefined!} diff --git a/desktop/flipper-plugin/src/ui/data-table/DataTable.tsx b/desktop/flipper-plugin/src/ui/data-table/DataTable.tsx index f270509dd..b283f2c70 100644 --- a/desktop/flipper-plugin/src/ui/data-table/DataTable.tsx +++ b/desktop/flipper-plugin/src/ui/data-table/DataTable.tsx @@ -69,7 +69,9 @@ interface DataTableBaseProps { tableManagerRef?: RefObject | undefined>; // Actually we want a MutableRefObject, but that is not what React.createRef() returns, and we don't want to put the burden on the plugin dev to cast it... onCopyRows?(records: T[]): string; onContextMenu?: (selection: undefined | T) => React.ReactElement; - onRenderEmpty?: null | ((dataSource?: DataSource) => React.ReactElement); + onRenderEmpty?: + | null + | ((dataSource?: DataSource) => React.ReactElement); } export type ItemRenderer = ( @@ -80,7 +82,7 @@ export type ItemRenderer = ( type DataTableInput = | { - dataSource: DataSource; + dataSource: DataSource; records?: undefined; recordsKey?: undefined; } @@ -558,7 +560,9 @@ DataTable.defaultProps = { } as Partial>; /* eslint-disable react-hooks/rules-of-hooks */ -function normalizeDataSourceInput(props: DataTableInput): DataSource { +function normalizeDataSourceInput( + props: DataTableInput, +): DataSource { if (props.dataSource) { return props.dataSource; } @@ -578,7 +582,10 @@ function normalizeDataSourceInput(props: DataTableInput): DataSource { } /* eslint-enable */ -function syncRecordsToDataSource(ds: DataSource, records: readonly T[]) { +function syncRecordsToDataSource( + ds: DataSource, + records: readonly T[], +) { const startTime = Date.now(); ds.clear(); // TODO: optimize in the case we're only dealing with appends or replacements diff --git a/desktop/flipper-plugin/src/ui/data-table/DataTableManager.tsx b/desktop/flipper-plugin/src/ui/data-table/DataTableManager.tsx index 883b187f1..0b41e72f8 100644 --- a/desktop/flipper-plugin/src/ui/data-table/DataTableManager.tsx +++ b/desktop/flipper-plugin/src/ui/data-table/DataTableManager.tsx @@ -93,7 +93,7 @@ type DataManagerActions = | Action<'setAutoScroll', {autoScroll: boolean}>; type DataManagerConfig = { - dataSource: DataSource; + dataSource: DataSource; defaultColumns: DataTableColumn[]; scope: string; onSelect: undefined | ((item: T | undefined, items: T[]) => void); @@ -279,11 +279,11 @@ export type DataTableManager = { toggleColumnVisibility(column: keyof T): void; sortColumn(column: keyof T, direction?: SortDirection): void; setSearchValue(value: string): void; - dataSource: DataSource; + dataSource: DataSource; }; export function createDataTableManager( - dataSource: DataSource, + dataSource: DataSource, dispatch: DataTableDispatch, stateRef: MutableRefObject>, ): DataTableManager { @@ -400,7 +400,7 @@ function addColumnFilter( } export function getSelectedItem( - dataSource: DataSource, + dataSource: DataSource, selection: Selection, ): T | undefined { return selection.current < 0 @@ -409,7 +409,7 @@ export function getSelectedItem( } export function getSelectedItems( - dataSource: DataSource, + dataSource: DataSource, selection: Selection, ): T[] { return [...selection.items] diff --git a/desktop/flipper-plugin/src/ui/data-table/TableContextMenu.tsx b/desktop/flipper-plugin/src/ui/data-table/TableContextMenu.tsx index 7a9cb068c..77f1ebcbb 100644 --- a/desktop/flipper-plugin/src/ui/data-table/TableContextMenu.tsx +++ b/desktop/flipper-plugin/src/ui/data-table/TableContextMenu.tsx @@ -26,7 +26,7 @@ import {textContent} from '../../utils/textContent'; const {Item, SubMenu} = Menu; export function tableContextMenuFactory( - datasource: DataSource, + datasource: DataSource, dispatch: DataTableDispatch, selection: Selection, columns: DataTableColumn[], diff --git a/desktop/flipper-plugin/src/utils/createTablePlugin.tsx b/desktop/flipper-plugin/src/utils/createTablePlugin.tsx index e116d2d84..1fb82c834 100644 --- a/desktop/flipper-plugin/src/utils/createTablePlugin.tsx +++ b/desktop/flipper-plugin/src/utils/createTablePlugin.tsx @@ -19,7 +19,7 @@ import {createDataSource} from '../state/createDataSource'; type PluginResult = { plugin(client: PluginClient>): { - rows: DataSource; + rows: DataSource; }; Component(): React.ReactElement; }; @@ -75,9 +75,9 @@ export function createTablePlugin< function plugin( client: PluginClient & Record, {}>, ) { - const rows = createDataSource([], { + const rows = createDataSource([], { persist: 'rows', - key: props.key, + key: props.key as keyof Row | undefined, }); const selection = createState(undefined); const isPaused = createState(false); diff --git a/desktop/plugins/public/network/types.tsx b/desktop/plugins/public/network/types.tsx index b24684f24..c67a4e32a 100644 --- a/desktop/plugins/public/network/types.tsx +++ b/desktop/plugins/public/network/types.tsx @@ -33,7 +33,7 @@ export interface Request { insights?: Insights; } -export type Requests = DataSource; +export type Requests = DataSource; export type SerializedRequest = Omit< Request,