Rewrite ItemList; update ItemList typings

[davwheat]

Fixes #3030

Changes proposed in this pull request:

• Converts the class to a generic
  • We now provide a typed ItemClass, so the .get(), .toArray(), etc return the correct type based on the contents of the ItemLIst
  • Extensions which use Typescript may need to modify their code to address new typing errors as a result. This does not break the code, however.
• New ItemList methods:
  • setContent() for setting the content of an item
  • setPriority() for setting the priority of an item
  • getPriority() for getting the priority of an item
• replace() is now deprecated
  • to set content, use the setContent(key, content) method
  • to set priority, use the setPriority(key, priority) method
• ItemList.items is now deprecated
  • instead, use toObject() to get a map of itemNames to { content, priority, itemName } records
• ItemList.toArray() has a new parameter: keepPrimitives
  • Setting to true will prevent content of primitive types, such as numbers/strings/etc., from being converted into Object representations of themselves
  • Setting to false (default) will retain the current behaviour of converting to Object-ified types
  • All object values, regardless of keepPrimitives, are proxied to provide read-only access to an itemName property.

Confirmed

• Frontend changes: tested on a local Flarum installation.
• Backend changes: tests are green (run composer test).

Required changes:

• Related documentation PR: (Remove if irrelevant)
• Related core extension PRs: (Remove if irrelevant)

[askvortsov1]

To me, "replace" implies a new content. Why not restrict "replace" to only do content, and have a separate "changePriority" method?

[davwheat]

Removing the functionality of changing priority from the replace function is definitely a breaking change.

If I wanted to add a changePriority method, I'd likely choose to have it just encapsulate the replace function:

changePriority = (key: string, priority: number) => this.replace(key, { _useNewSyntax: true, priority });

I think of replace as replacing the entire item as a new one, rather than just the content. Maybe this needs some further discussion?

I also think that being able to replace both the content and priority at once is nicer than doing two separate function calls with the same key.

[askvortsov1]

I think of replace as replacing the entire item as a new one, rather than just the content.

Well, what does "entire item" mean? I would argue that priority is moreso a property of the relation between items and their container than a property of the item itself. When someone asks how they can replace something with something else, that generally implies replacing in place, but keeping the same order. On a behavioral level, I think "replace" and "rearrange" make sense as separate, simple operations. Would be happy to hear counterarguments though.

I also think that being able to replace both the content and priority at once is nicer than doing two separate function calls with the same key.

I think that chained method calls alleviate this a bit.

Absolutely agree that we shouldn't remove this functionality until 2.0. We could deprecate it though?

[davwheat]

Good idea. So...

• replace will replace content with deprecated support for priority replacement
• setPriority (or another name?) will set the priority of the current key

Part of me wonders if we can take a jQuery approach and have something like items.select('key').replace('content').setPriority(10) 🤔

[askvortsov1]

Good idea. So...

• replace will replace content with deprecated support for priority replacement
• setPriority (or another name?) will set the priority of the current key

Part of me wonders if we can take a jQuery approach and have something like items.select('key').replace('content').setPriority(10) 🤔

Let's start with the first part, and look into select later. Prioritization!

[askvortsov1]

Good idea. So...

• replace will replace content with deprecated support for priority replacement
• setPriority (or another name?) will set the priority of the current key

Part of me wonders if we can take a jQuery approach and have something like items.select('key').replace('content').setPriority(10) 🤔

Let's start with the first part, and look into select later. Prioritization!

[askvortsov1]

I'm looking over this again, and while I'm a fan of most of the changes (especially typing!), toObject and toArray seem way, way too complex for me

• This proxy prevents direct modification of the internal state to prevent people breaking the class.

Why can't we just return an object of key => content? I don't think we should expose any internal state. This also lets us avoid all the complicated proxy stuff. On a side note, proxies are powerful, but we shouldn't use them unless we absolutely have to: it's a lot of added complexity.

.toArray()'s items are now proxied, meaning that values which were objects are no longer modified to add the itemName property. This is now provided through the Proxy.

If we're returning an array of primitives, we shouldn't have an itemName at all, so proxying is unnecessary. If we're returning an array of objects, we can use spread syntax to clone the object and cleanly attach the itemName. Proxies would be best to avoid.

[askvortsov1]

I don't think we should consider this to be part of the public API. Do we even need to set this on the internal representation?

[davwheat]

We don't use it internally, no. There's a chance someone somewhere uses it, but if you think we should just remove it, I'm fine with that.

[askvortsov1]

Let's remove it.

[davwheat]

Why can't we just return an object of key => content?

Because we need to be able to access the internal state of Items to correctly merge() two ItemLists.

If we're returning an array of primitives, we shouldn't have an itemName at all

We don't. itemName is added to object values. Primitives do not receive the itemName proxied property.

If we have an ItemList of translations (they are either strings or string arrays), then we have an ItemList<string | string[]>.

const items = new ItemList<string | string[]>();

items.add('a', app.translator.trans('key1'), 10); // 'test'
items.add('b', app.translator.trans('key2', { test: 'hello' }), 9); // ['test', 'hello']

items.toArray();
// [ String('test') { itemName: 'key1' }, Array ['test', 'hello'] { itemName: 'key2' } ]

items.toArray(false);
// [ 'test', Array ['test', 'hello'] { itemName: 'key2' } ]

we can use spread syntax to clone the object

That will work fine for maps, but not other objects. If our ItemList holds dates, for example, all the date info is completely stripped when cloning with spread. It renders the ItemList completely useless.

We only proxy the content, as that is what is returned in the array. If we use spread to clone the Item object, then adding itemName to the content will end up mutating the internal state which is exactly what you said we should avoid.

let x = new Date();
let y = { ...x, itemName: 'date1' };

console.log(y);
// { itemName: 'date1' }

[askvortsov1]

Because we need to be able to access the internal state of Items to correctly merge() two ItemLists.

I still don't feel right about this: maybe we should tsignore and use the private _items property directly, since the caller is within the ItemList code? Maybe a (as this) or something case could help there?

I'm starting to think that the whole itemName-being-set approach might not be the best way to do this: what if instead, we returned an array of [itemName, content] tuples? I realize this would be a breaking change so we couldn't do it yet, but in the long term it would be a lot less hacky.

[davwheat]

what if instead, we returned an array of [itemName, content] tuples?

I'm not sure which way I'd prefer to do this:

return { itemNames: [ 'item1', 'item2', 'item3' ], items: [ <Item1 />, <Item2 />, <Item3 /> ] }

...or as tuples.

This way is more similar to the current interface, in the sense that we can do const { items } = itemList.toArray() without worrying about map-ing an array of tuples to just the content to render it.

[askvortsov1]

what if instead, we returned an array of [itemName, content] tuples?

I'm not sure which way I'd prefer to do this:

return { itemNames: [ 'item1', 'item2', 'item3' ], items: [ <Item1 />, <Item2 />, <Item3 /> ] }

...or as tuples.

This way is more similar to the current interface, in the sense that we can do const { items } = itemList.toArray() without worrying about map-ing an array of tuples to just the content to render it.

Now that I think about it, we cant do either because BC. Let's keep the proxy stuff for now, and hope we eventually come up with something better

[askvortsov1]

I still don't feel right about this: maybe we should tsignore and use the private _items property directly, since the caller is within the ItemList code? Maybe a (as this) or something case could help there?

Bump on this btw

[askvortsov1]

Doesn't this lose priority though?

[davwheat]

No, it's copying the whole Item class from the other ItemList and adding it to this one.

[davwheat]

Rebased onto latest master.

[davwheat]

I still don't feel right about this: maybe we should tsignore and use the private _items property directly, since the caller is within the ItemList code? Maybe a (as this) or something case could help there?

Turns out TS is more than fine with accessing private properties as long as its within the ItemClass' implementation of itself.

[davwheat]

Ok, I've updated the OP again to contain my latest changes. I think I'm pretty settled on these and the new API overall. It should be backwards compatible apart from:

• ItemList.items cannot be modified directly to add/remove items
• Item.key is no longer present. This could affect ordering for some ItemLists where multiple Items had the same priority, and may break