From b4f39520bdb10a83182dfc67faedc7b225b45242 Mon Sep 17 00:00:00 2001 From: David Wheatley Date: Thu, 11 Nov 2021 20:01:10 +0000 Subject: [PATCH] Rewrite ItemList; update ItemList typings (#3005) * Improve typings for ItemList * Add new `.replace()` syntax * Update JSDoc * Add missing `T` type * Fix typo Co-authored-by: Alexander Skvortsov <38059171+askvortsov1@users.noreply.github.com> * Allow choice to not set `itemName` property when calling `toArray` * Make `ItemList.items` read-only * Modify `.replace()`; add `.changePriority()` * Complete rename * Update JSDoc * Add `.toObject()` method; deprecate `.items` * Rewrite `.toArray()` to use Proxy instead of modifying the item content - Fixes #3030 - Fixes issue where setting `itemName` property could result in errors depending on the object type (e.g. proxies) - Fixes unneeded duplication of item list - Add option to disable setting `itemName` property on primitives * Simplify condition * Remove debug code * Make proxying function protected instead of private * Update a usage of ItemList as an example * Make `itemName` property read-only * Use correct capitalisation of `object` * Invert `toArray` parameter function * Simplify isEmpty check * Update ItemList.ts * Fix `merge()` * Remove extra JSDoc comment * Use `._items` directly for merging * Rename methods: `replace` -> `set`; `changePriority` -> `setPriority` This more closely matches our existing method names (`get()`) * Change `items` getter * Simplify proxying * Update URL to source function * Update compat * Various changes to toObject * Remove `Item.key` * Make item content proxy method private * Enforce merge typings * Update TSDoc comments to use `{@link}` for references to methods * Correct references to deprecated `.replace` method * Throw error when setting content/priority of non-existent items * Remove intermediary variable * Update TSDoc block * Update js/src/@types/global.d.ts Co-authored-by: Alexander Skvortsov <38059171+askvortsov1@users.noreply.github.com> --- framework/core/js/src/@types/global.d.ts | 5 + .../js/src/admin/components/UserListPage.tsx | 5 +- framework/core/js/src/common/compat.js | 2 + .../core/js/src/common/utils/ItemList.ts | 313 ++++++++++++++---- .../core/js/src/common/utils/isObject.ts | 27 ++ 5 files changed, 291 insertions(+), 61 deletions(-) create mode 100644 framework/core/js/src/common/utils/isObject.ts diff --git a/framework/core/js/src/@types/global.d.ts b/framework/core/js/src/@types/global.d.ts index ec54dab6c..382d32d5d 100644 --- a/framework/core/js/src/@types/global.d.ts +++ b/framework/core/js/src/@types/global.d.ts @@ -1,3 +1,8 @@ +declare type Writable = { -readonly [P in keyof T]: T[P] }; +declare type DeepWritable = { -readonly [P in keyof T]: DeepWritable }; + +declare type DeepReadonly = { readonly [P in keyof T]: DeepReadonly }; + /** * UTILITY TYPES */ diff --git a/framework/core/js/src/admin/components/UserListPage.tsx b/framework/core/js/src/admin/components/UserListPage.tsx index a4cfbc7e8..7005617a7 100644 --- a/framework/core/js/src/admin/components/UserListPage.tsx +++ b/framework/core/js/src/admin/components/UserListPage.tsx @@ -14,6 +14,7 @@ import classList from '../../common/utils/classList'; import extractText from '../../common/utils/extractText'; import AdminPage from './AdminPage'; +import Mithril from 'mithril'; type ColumnData = { /** @@ -97,7 +98,7 @@ export default class UserListPage extends AdminPage { ]; } - const columns: (ColumnData & { itemName: string })[] = this.columns().toArray(); + const columns = this.columns().toArray(); return [

{app.translator.trans('core.admin.users.total_users', { count: this.userCount })}

, @@ -177,7 +178,7 @@ export default class UserListPage extends AdminPage { * * See `UserListPage.tsx` for examples. */ - columns(): ItemList { + columns(): ItemList { const columns = new ItemList(); columns.add( diff --git a/framework/core/js/src/common/compat.js b/framework/core/js/src/common/compat.js index 92e9fd41a..d17bd84cc 100644 --- a/framework/core/js/src/common/compat.js +++ b/framework/core/js/src/common/compat.js @@ -79,6 +79,7 @@ import listItems from './helpers/listItems'; import Fragment from './Fragment'; import DefaultResolver from './resolvers/DefaultResolver'; import PaginatedListState from './states/PaginatedListState'; +import isObject from './utils/isObject'; export default { extend: extend, @@ -114,6 +115,7 @@ export default { 'utils/mapRoutes': mapRoutes, 'utils/withAttr': withAttr, 'utils/throttleDebounce': ThrottleDebounce, + 'utils/isObject': isObject, 'models/Notification': Notification, 'models/User': User, 'models/Post': Post, diff --git a/framework/core/js/src/common/utils/ItemList.ts b/framework/core/js/src/common/utils/ItemList.ts index 9cf51677e..3f7857601 100644 --- a/framework/core/js/src/common/utils/ItemList.ts +++ b/framework/core/js/src/common/utils/ItemList.ts @@ -1,9 +1,16 @@ -class Item { - content: any; - priority: number; - key?: number; +import isObject from './isObject'; - constructor(content: any, priority?: number) { +export interface IItemObject { + content: T; + itemName: string; + priority: number; +} + +class Item { + content: T; + priority: number; + + constructor(content: T, priority: number) { this.content = content; this.priority = priority; } @@ -13,37 +20,56 @@ class Item { * The `ItemList` class collects items and then arranges them into an array * by priority. */ -export default class ItemList { +export default class ItemList { /** - * The items in the list + * The items in the list. */ - items: { [key: string]: Item } = {}; + protected _items: Record> = {}; + + // TODO: [Flarum 2.0] Remove `.items` getter. + /** + * A **read-only copy** of items in the list. + * + * We don't allow adding new items to the ItemList via setting new properties, + * nor do we allow modifying existing items directly. + * + * @deprecated Use {@link ItemList.toObject} instead. + */ + get items(): DeepReadonly>> { + return new Proxy(this._items, { + set() { + console.warn('Modifying `ItemList.items` is not allowed.'); + return false; + }, + }); + } /** * Check whether the list is empty. */ isEmpty(): boolean { - for (const i in this.items) { - if (this.items.hasOwnProperty(i)) { - return false; - } - } - - return true; + return Object.keys(this._items).length === 0; } /** * Check whether an item is present in the list. */ has(key: string): boolean { - return !!this.items[key]; + return Object.keys(this._items).includes(key); } /** * Get the content of an item. */ - get(key: string): any { - return this.items[key].content; + get(key: string): T { + return this._items[key].content; + } + + /** + * Get the priority of an item. + */ + getPriority(key: string): number { + return this._items[key].priority; } /** @@ -51,29 +77,105 @@ export default class ItemList { * * @param key A unique key for the item. * @param content The item's content. - * @param [priority] The priority of the item. Items with a higher - * priority will be positioned before items with a lower priority. + * @param priority The priority of the item. Items with a higher priority + * will be positioned before items with a lower priority. */ - add(key: string, content: any, priority: number = 0): this { - this.items[key] = new Item(content, priority); + add(key: string, content: T, priority: number = 0): this { + this._items[key] = new Item(content, priority); + + return this; + } + + // TODO: [Flarum 2.0] Remove deprecated `.replace()` method. + /** + * Replace an item and/or priority in the list, only if it is already present. + * + * If `content` or `priority` are `null`, these values will not be replaced. + * + * If the provided `key` is not present, nothing will happen. + * + * @deprecated Please use the {@link ItemList.setContent} and {@link ItemList.setPriority} + * methods to replace items and their priorities. This method will be removed in Flarum 2.0. + * + * @param key The key of the item in the list + * @param content The item's new content + * @param priority The item's new priority + * + * @example Replace priority and not content. + * items.replace('myItem', null, 10); + * + * @example Replace content and not priority. + * items.replace('myItem',

My new value.

); + * + * @example Replace content and priority. + * items.replace('myItem',

My new value.

, 10); + */ + replace(key: string, content: T | null = null, priority: number | null = null): this { + if (!this.has(key)) return this; + + if (content !== null) { + this._items[key].content = content; + } + + if (priority !== null) { + this._items[key].priority = priority; + } return this; } /** - * Replace an item in the list, only if it is already present. + * Replaces an item's content, if the provided item key exists. + * + * If the provided `key` is not present, nothing will happen. + * + * @param key The key of the item in the list + * @param content The item's new content + * + * @example Replace item content. + * items.setContent('myItem',

My new value.

); + * + * @example Replace item content and priority. + * items + * .setContent('myItem',

My new value.

) + * .setPriority('myItem', 10); + * + * @throws If the provided `key` is not present in the ItemList. */ - replace(key: string, content: any = null, priority: number = null): this { - if (this.items[key]) { - if (content !== null) { - this.items[key].content = content; - } - - if (priority !== null) { - this.items[key].priority = priority; - } + setContent(key: string, content: T): this { + if (!this.has(key)) { + throw new Error(`[ItemList] Cannot set content of Item. Key \`${key}\` is not present.`); } + // Saves on bundle size to call the deprecated method internally + return this.replace(key, content); + } + + /** + * Replaces an item's priority, if the provided item key exists. + * + * If the provided `key` is not present, nothing will happen. + * + * @param key The key of the item in the list + * @param priority The item's new priority + * + * @example Replace item priority. + * items.setPriority('myItem', 10); + * + * @example Replace item priority and content. + * items + * .setPriority('myItem', 10) + * .setContent('myItem',

My new value.

); + * + * @throws If the provided `key` is not present in the ItemList. + */ + setPriority(key: string, priority: number): this { + if (!this.has(key)) { + throw new Error(`[ItemList] Cannot set priority of Item. Key \`${key}\` is not present.`); + } + + this._items[key].priority = priority; + return this; } @@ -81,51 +183,144 @@ export default class ItemList { * Remove an item from the list. */ remove(key: string): this { - delete this.items[key]; + delete this._items[key]; return this; } /** * Merge another list's items into this one. + * + * The list passed to this function will overwrite items which already exist + * with the same key. */ - merge(items: this): this { - for (const i in items.items) { - if (items.items.hasOwnProperty(i) && items.items[i] instanceof Item) { - this.items[i] = items.items[i]; + merge(otherList: ItemList): ItemList { + Object.keys(otherList._items).forEach((key) => { + const val = otherList._items[key]; + + if (val instanceof Item) { + this._items[key] = val; } - } + }); return this; } /** - * Convert the list into an array of item content arranged by priority. Each - * item's content will be assigned an `itemName` property equal to the item's - * unique key. + * Convert the list into an array of item content arranged by priority. + * + * This **does not** preserve the original types of primitives and proxies + * all content values to make `itemName` accessible on them. + * + * **NOTE:** If your ItemList holds primitive types (such as numbers, booleans + * or strings), these will be converted to their object counterparts if you do + * not provide `true` to this function. + * + * **NOTE:** Modifying any objects in the final array may also update the + * content of the original ItemList. + * + * @param keepPrimitives Converts item content to objects and sets the + * `itemName` property on them. + * + * @see https://github.com/flarum/core/issues/3030 */ - toArray(): any[] { - const items: Item[] = []; + toArray(keepPrimitives?: false): (T & { itemName: string })[]; + /** + * Convert the list into an array of item content arranged by priority. + * + * Content values that are already objects will be proxied and have + * `itemName` accessible on them. Primitive values will not have the + * `itemName` property accessible. + * + * **NOTE:** Modifying any objects in the final array may also update the + * content of the original ItemList. + * + * @param keepPrimitives Converts item content to objects and sets the + * `itemName` property on them. + */ + toArray(keepPrimitives: true): (T extends object ? T & Readonly<{ itemName: string }> : T)[]; - for (const i in this.items) { - if (this.items.hasOwnProperty(i) && this.items[i] instanceof Item) { - this.items[i].content = Object(this.items[i].content); + toArray(keepPrimitives: boolean = false): T[] | (T & Readonly<{ itemName: string }>)[] { + const items: Item[] = Object.keys(this._items).map((key, i) => { + const item = this._items[key]; - this.items[i].content.itemName = i; - items.push(this.items[i]); - this.items[i].key = items.length; + if (!keepPrimitives || isObject(item.content)) { + // Convert content to object, then proxy it + return { + ...item, + content: this.createItemContentProxy(isObject(item.content) ? item.content : Object(item.content), key), + }; + } else { + // ...otherwise just return a clone of the item. + return { ...item }; } - } + }); - return items - .sort((a, b) => { - if (a.priority === b.priority) { - return a.key - b.key; - } else if (a.priority > b.priority) { - return -1; + return items.sort((a, b) => b.priority - a.priority).map((item) => item.content); + } + + /** + * A read-only map of all keys to their respective items in no particular order. + * + * We don't allow adding new items to the ItemList via setting new properties, + * nor do we allow modifying existing items directly. You should use the + * {@link ItemList.add}, {@link ItemList.setContent} and + * {@link ItemList.setPriority} methods instead. + * + * To match the old behaviour of the `ItemList.items` property, call + * `Object.values(ItemList.toObject())`. + * + * @example + * const items = new ItemList(); + * items.add('b', 'My cool value', 20); + * items.add('a', 'My value', 10); + * items.toObject(); + * // { + * // a: { content: 'My value', priority: 10, itemName: 'a' }, + * // b: { content: 'My cool value', priority: 20, itemName: 'b' }, + * // } + */ + toObject(): DeepReadonly>> { + return Object.keys(this._items).reduce((map, key) => { + const obj = { + content: this.get(key), + itemName: key, + priority: this.getPriority(key), + }; + + map[key] = obj; + + return map; + }, {} as Record>); + } + + /** + * Proxies an item's content, adding the `itemName` readonly property to it. + * + * @example + * createItemContentProxy({ foo: 'bar' }, 'myItem'); + * // { foo: 'bar', itemName: 'myItem' } + * + * @param content The item's content (objects only) + * @param key The item's key + * @returns Proxied content + * + * @internal + */ + private createItemContentProxy(content: C, key: string): Readonly { + return new Proxy(content, { + get(target, property, receiver) { + if (property === 'itemName') return key; + + return Reflect.get(target, property, receiver); + }, + set(target, property, value, receiver) { + if (key !== null && property === 'itemName') { + throw new Error('`itemName` property is read-only'); } - return 1; - }) - .map((item) => item.content); + + return Reflect.set(target, property, value, receiver); + }, + }) as C & { itemName: string }; } } diff --git a/framework/core/js/src/common/utils/isObject.ts b/framework/core/js/src/common/utils/isObject.ts new file mode 100644 index 000000000..4d01826b7 --- /dev/null +++ b/framework/core/js/src/common/utils/isObject.ts @@ -0,0 +1,27 @@ +/** + * Returns if the passed value is an object. + * + * In this context, "object" refers to **any non-primitive value**, including + * arrays, function, maps, dates, and more. + * + * @example + * isObject({}); // true + * @example + * isObject([]); // true + * @example + * isObject(function () {}); // true + * @example + * isObject(Object(1)); // true + * @example + * isObject(null); // false + * @example + * isObject(1); // false + * @example + * isObject("hello world"); // false + * + * @see https://github.com/jashkenas/underscore/blob/943977e34e2279503528a71ddcc2dd5f96483945/underscore.js#L87-L91 + */ +export default function isObject(obj: unknown): obj is object { + const type = typeof obj; + return type === 'function' || (type === 'object' && !!obj); +}