Proposal: Better SchemaPiece options

kyranet commented 6 years ago

Describe the proposal

Since SGv1, in the PR #255 from Komada, we had the first implementation of SchemaPiece being able to store more than a single value of the same type (instead of being type array, from the former configuration system).

However, this system is extremely old and should be renamed and refactored into something much better, in this proposal, I'll use SchemaPiece#multiValue as a placeholder for a better name.

What does SchemaPiece#multiValue do compared to SchemaPiece#array? The current name limits the options, the values may be contained inside an array or not, but, what if I want to use Set or Map? The current system does not allow it, but multiValue will.

Implementation

There are multiple ways to achieve this, but I'll present two in this proposal:

ValueController

This is more versatile, it allows custom types and moves some of the heavy work from SG to them, it also allows users to implement their own ValueController, for example, one being Collection extending Map and so on. Compatibility with string for simplicity in SchemaFolderAddOptions is accepted with a later resolve.

Said ValueController would implement basic methods such as create, clone, add, remove and modify and they could be stored in GatewayDriver together with the available key types.

Internal Resolving

This is less customizable and more hardcoded, plus makes SettingGateway's internals much heavier. These methods would be integrated in a different file (similar to a util) for later usage.

In both options, you'll be able to do

SchemaFolder#add('tags', { type: 'string', multiValue: 'Map' });

And the new values will be stored in the Map. However, some of the multiValue may not be able to be directly configurable, so they'll have the same behavior as type: 'any': sets SchemaPiece#configurable to false.

This proposal is not merging on #179 but in a later one.

UnseenFaith commented 6 years ago

The example of using set in this is completely useless because Array already removes/adds by default, which is the equivalent to using new Set(). As for map you have issues:

what should you do about storing discord.js structures (flattening to id and resolving on sync)
what about non-discord.js structures that don't have a id value (storing a complete json is going to get pretty overkill as the map increases in size

kyranet commented 6 years ago

Not quite. Set#has() is faster than most methods for Array. And there can never be two entries with the same value (like in Array) but it benefits on how easy it is to use. SettingGateway would build an instance of Set instead of Array when the key requires a Set.
That's an issue for ValueController, not for the proposal itself, although that can be handled, it's up to the developer to not do such practises.
In case of Map, it's unaffected. Why? Because one simple fact: I cannot store hashed collections (Map/WeakMap/Set/WeakSet) into a database, I need to convert them to array first.

For example, the following Map instance:

Map { '224236171838881792' => { colour: 'red', animal: 'dog' } }

Is stored into a JSON db as:

[
    ['224236171838881792', { colour: 'red', animal: 'dog' }]
]

Then if you pass the array above into new Map(), you'll get the first map. And it does not have a property of id, instead, it's mapped by a key that serves as id.

UnseenFaith commented 6 years ago

Why can't you just do an object instead of map then

{
  '224236171838881792': { somedata }
}

And again, Array already does what Set does (only having one key of whatever) along with the ability to duplicate entries as well, don't think there's a reason you need to have functionality in both, so Array default adding/removing should probably be removed

More edits because im stupid and i don't think my comments through: It's also probably more performant to do an object type instead of map, considering once you parse the returned JSON you will have your object, instead of having to also iterate through that entire array just to get the items set in the map

kyranet commented 6 years ago

Because that requires the iteration of an object's keys with Object.keys() which performance is 27 times lower than iterating an array by index. Building so many Configuration instances and needing to run Object.keys in one or more keys in every single one of them may result on a slow startup.

Outside performance, it's a pain for TypeScript users to handle that, and it's also an antipattern, it should not be made.

UnseenFaith commented 6 years ago

TypeScript has no bearing on this conversation. realistically this is only going to be used by Javascript users.

It's already going to be taxing on performance and startup for having to iterate over the parsed array to create the map.

It's also going to create a new interface that will be more ambiguous compared to the other ones

<Config>.someMapKey.get() /* <-- having to use a function to get the data rather then */
<Config>.someObjectKey["Object"]
<Config>.someArrayKey[0]
<Config>.someString

Nonetheless, I do think a "Type" Vault is necessary so custom types are easier to add.

kyranet commented 6 years ago

It's not taxing on performance if the array is empty, which it's the case for the 50% of cases. new Map() doesn't iterate the array if it's empty. Unlike your idea of an object, that needs to run an expensive function (which filters ~7 built-in properties of Object, so it always has keys to iterate).

Not to mention that when you add/remove a property from an object, you are rebuilding it from scratch, so, it's an incredibly bad idea, and remember that we have discussed about this in Komada 0.20.0 development. This behavior does not happen in a Map as it's a collection of key/value designed to have a variable amount of entries, which objects aren't (due to their nature of immutability).

What you're proposing is much more taxing than my proposal in both CPU and RAM, as well as it contains antipatterns that seriously affect the performance in V8, it's all based on program optimization.

EDIT:

A "type" vault is exactly the type any. it allows any type, with no restrictions, the multiValue doesn't wrap the data from the DB into an array unless told so, what it does is wrap that data in a different constructor, such as Map, Set, Array or a custom one.

dirigeants / klasa