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

@@ -1,3 +1,8 @@
+declare type Writable<T> = { -readonly [P in keyof T]: T[P] };
+declare type DeepWritable<T> = { -readonly [P in keyof T]: DeepWritable<T[P]> };
+
+declare type DeepReadonly<T> = { readonly [P in keyof T]: DeepReadonly<T[P]> };
+
 /**
  * UTILITY TYPES
  */

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 [
       <p class="UserListPage-totalUsers">{app.translator.trans('core.admin.users.total_users', { count: this.userCount })}</p>,
@@ -177,7 +178,7 @@ export default class UserListPage extends AdminPage {
    *
    * See `UserListPage.tsx` for examples.
    */
-  columns(): ItemList {
+  columns(): ItemList<ColumnData> {
     const columns = new ItemList();
 
     columns.add(

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,

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<T> {
+  content: T;
+  itemName: string;
+  priority: number;
+}
+
+class Item<T> {
+  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<T> {
   /**
-   * The items in the list
+   * The items in the list.
    */
-  items: { [key: string]: Item } = {};
+  protected _items: Record<string, Item<T>> = {};
+
+  // 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<Record<string, Item<T>>> {
+    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 <caption>Replace priority and not content.</caption>
+   * items.replace('myItem', null, 10);
+   *
+   * @example <caption>Replace content and not priority.</caption>
+   * items.replace('myItem', <p>My new value.</p>);
+   *
+   * @example <caption>Replace content and priority.</caption>
+   * items.replace('myItem', <p>My new value.</p>, 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 <caption>Replace item content.</caption>
+   * items.setContent('myItem', <p>My new value.</p>);
+   *
+   * @example <caption>Replace item content and priority.</caption>
+   *          items
+   *            .setContent('myItem', <p>My new value.</p>)
+   *            .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 <caption>Replace item priority.</caption>
+   * items.setPriority('myItem', 10);
+   *
+   * @example <caption>Replace item priority and content.</caption>
+   *          items
+   *            .setPriority('myItem', 10)
+   *            .setContent('myItem', <p>My new value.</p>);
+   *
+   * @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<T>): ItemList<T> {
+    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<T>[] = 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<Record<string, IItemObject<T>>> {
+    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<string, IItemObject<T>>);
+  }
+
+  /**
+   * 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<C extends object>(content: C, key: string): Readonly<C & { itemName: string }> {
+    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 };
   }
 }

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);
+}