adventureland-community / typed-adventureland

9 stars 4 forks source link

Types for the `G` data object. #1

Closed Telokis closed 1 year ago

Telokis commented 2 years ago

The G object is huge but very important. We need its types to be precise and easy to use.

The Global object itself

This object contains several properties but they are not directly related. Due to this fact, I think exhaustively listing each property ourselves would be the preferred solution:

type GObject = {
    positions: /* ... */;
    titles: /* ... */;
    tilesets: /* ... */;
    images: /* ... */;
    imagesets: /* ... */;
    dimensions: /* ... */;
    emotions: /* ... */;
    multipliers: /* ... */;
    maps: /* ... */;
    version: /* ... */; // <-- This is a number
    cosmetics: /* ... */;
    conditions: /* ... */;
    monsters: /* ... */;
    achievements: /* ... */;
    docs: /* ... */;
    dismantle: /* ... */;
    projectiles: /* ... */;
    tokens: /* ... */;
    craft: /* ... */;
    animations: /* ... */;
    npcs: /* ... */;
    geometry: /* ... */;
    items: /* ... */;
    levels: /* ... */;
    events: /* ... */;
    skills: /* ... */;
    classes: /* ... */;
    games: /* ... */;
    sets: /* ... */;
    drops: /* ... */;
    sprites: /* ... */;
};

Most of the fields inside this object are objects themselves and follow the pattern where

key   = Name of the thing (item name, monster name, map name, ...)
value = Description of the thing

Fields names

To have exhaustive types for this kind of fields, we might want to have all names available as types.
Something like the following:

// Union of all existing items names.
type ItemName = "harbringer" | "dexamulet" | "tigerhelmet" /* | ... */;

We could then use these types in the big GObject type definition as such:

type GObject = {
    // ...
    items: Record<ItemName, /* Type for G.items values */>;
    // ...
};

Fields values

In order to have useful types, it will probably be necessary to try and group the different possibles values when it makes sense. For example, the items object will contain lots of different unrelated types of objects: equipments will have fields related to stats, elixirs will have a duration property and so on.
Before we can try and find common fields, it will probably make sense to group values using a relevant property. For example, items have the type field that could be the first filter.
Creating a dedicated type for each possible group would give us more flexibility and it would allow us to give more meaning to the types themselves. (Instead of a huge GItem type, we would have GWeaponItem, GElixirItem, and so on.)

Once the grouping is defined, we need to perform a little bit of analysis on the fields in each group. There can be four cases:

Based on these four cases and the resulting tuple, we can properly type the field.

Telokis commented 2 years ago

Here a very quick and dirty analysis function:

function groupBy(obj, key) {
    const sorted = {};
    const entries = Object.entries(obj);

    for (const [k, v] of entries) {
        if (!(key in v)) {
            throw new Error(`Key ${key} not found in ${k}.`);
        }

        const filterValue = v[key];

        if (!(filterValue in sorted)) {
            sorted[filterValue] = [];
        }

        sorted[filterValue].push([k, v]);
    }

    return Object.fromEntries(Object.entries(sorted).map(([k, v]) => [k, Object.fromEntries(v)]));
}

function analyzeFields(arr) {
    const seenFields = new Set(arr.flatMap((o) => Object.keys(o)));
    const fields = Object.fromEntries(
        [...seenFields.values()].map((n) => [n, { optional: false, types: new Set([]) }])
    );

    for (const obj of arr) {
        for (const seenField of seenFields) {
            if (!(seenField in obj)) {
                fields[seenField].optional = true;
                continue;
            }

            const type = typeof obj[seenField];
            fields[seenField].types.add(type);
        }
    }

    return fields;
}

const groupedItems = groupBy(items, "type");

console.log(groupedItems.cscroll);
console.log(analyzeFields(Object.values(groupedItems.cscroll)));

Here is the output of groupedItems.cscroll:

{
  cscroll0: {
    name: 'Compound Scroll',
    g: 6400,
    grade: 0,
    explanation: 'Scroll to combine 3 accessories. Things get challenging after +1. If the combination fails, the item is lost.',
    s: 9999,
    skin: 'cscroll0',
    type: 'cscroll'
  },
  cscroll1: {
    name: 'High Compound Scroll',
    g: 240000,
    grade: 1,
    explanation: 'Scroll to combine 3 high grade accessories. If the combination fails, the item is lost.',
    s: 9999,
    skin: 'cscroll1',
    type: 'cscroll'
  },
  cscroll2: {
    name: 'Rare Compound Scroll',
    g: 9200000,
    grade: 2,
    explanation: 'Scroll to combine 3 rare accessories. If the combination fails, the item is lost.',
    s: 9999,
    skin: 'cscroll2',
    type: 'cscroll'
  },
  cscroll3: {
    a: true,
    s: 9999,
    name: 'Legendary Compound Scroll',
    g: 1840000000,
    skin: 'cscroll3',
    grade: 3,
    explanation: "A mysterious compound scroll, you can feel that it's very powerful. If the combination fails, the item is lost.",
    type: 'cscroll',
    markup: 20
  }
}

Here is the analysis result:

{
    name: {
        optional: false,
        types: ["string"],
    },
    g: {
        optional: false,
        types: ["number"],
    },
    grade: {
        optional: false,
        types: ["number"],
    },
    explanation: {
        optional: false,
        types: ["string"],
    },
    s: {
        optional: false,
        types: ["number"],
    },
    skin: {
        optional: false,
        types: ["string"],
    },
    type: {
        optional: false,
        types: ["string"],
    },
    a: {
        optional: true,
        types: ["boolean"],
    },
    markup: {
        optional: true,
        types: ["number"],
    },
};
thmsndk commented 2 years ago

Having the item names as a type is definitely something that is a good idea. the current types already has it. I have just made an initial script to generate them for items, I think we should keep them in a generated folder, so we don't change the data inside there and can always regenerate them for when new versions of data.js are added.

They are currently generated by running npx ts-node data-to-types.ts in the console. The script is located here https://github.com/adventureland-community/typed-adventureland/blob/feature/generate-dts-files/data-to-types.ts

And what it generated can be seen here. https://github.com/adventureland-community/typed-adventureland/tree/feature/generate-dts-files/src/generated

We might want to customize some types additionally, the name CscrollName for example I think is a terrible name, instead of say CompoundScroll

We might also want to split out PotName into health and mana potion types.

If you want to generate more specific types based on the content of G.items and the other entries we can do that. But I think we will need to handcraft a lot of types as well to get the behavior and code experience we want to accomplish.

Especially if we want to document the different properties.

All in all, I agree with all your points of the issue. I definitely want to have more specific types for GItems as you mention.

Generating ItemName, MonsterName, MapName, and so forth is something I think we should be doing. and then using thoose in the different "advanced" types we make :)

thmsndk commented 2 years ago

I have extracted out G into G.ts if you want to start on changing the types around based on your analysis. I've commented out some of the types that have not been ported to a decent place yet. You can see it here https://github.com/adventureland-community/typed-adventureland/blob/feature/generate-dts-files/src/G.ts

thmsndk commented 2 years ago

Perhaps persisting the analysis result in a folder allowing us to run the analysis later with a compare of the previous result would help in detecting changes on fields / property types?

Telokis commented 2 years ago

I'm wondering what the real-world use case would be for distinct names types. Do you use the PotionName specifically, for example?

thmsndk commented 2 years ago

I'm wondering what the real-world use case would be for distinct names types. Do you use the PotionName specifically, for example?

In my Potion handling code I've typed HealthPotion and ManaPotion specifically. Because I have one function to handle health, and another to handle mana. If new health potions are added my array definition would whine if I'm not handling the new potion type

It could also help with narrowing down types for certain things, specific character skills for example.

Telokis commented 2 years ago

Ok, I see what you mean. It's interesting but I'm a bit afraid we could overwhelm the user with too many types.
But I can't guess that, we'll have to try and see how it goes.

thmsndk commented 2 years ago

As long as we generate a union type of all of them, they can still use that though 🤔 https://github.com/adventureland-community/typed-adventureland/blob/d444a528c83b9b9c213efcb00fcf0908ebda9507/src/generated/item-names.ts#L620-L624

Telokis commented 2 years ago

I know but there is also the issue of discoverability that you mentioned previously.
All types will always be shown by VSCode for autocomplete. A new user might not become aware of ItemName if it's in the middle of several other item-related types.
Moreover, even though I can see the use-case for narrowly typing when it comes to items, we might be faced with situations where we have to cast even though we know the type is right.

I think it makes sense to have one type per item type but I feel like being more precise will bring more confusion than comfort. One of the reasons being that an amulet will be entirely different from an elixir, they won't have common properties at all. On the other hand, a health potion will behave exactly the same as a mana potion and everywhere one is used, the other would also work fine.

thmsndk commented 2 years ago

The readme could detail the import types like ItemName was my thought. But you would also learn about a lot of the common types when looking at functions that accepts the type.

But yeah perhaps we don't have to have precise types for potions, as all potions "behave" the same way.

I am a little unsure about what you are suggesting to do though.

Telokis commented 2 years ago

I do agree, the README should detail most of the types, yes.

My message mainly was me thinking out loud. I still think it's nice to have one union per item type. Because we will probably have something like one interface per item type anyway.

Telokis commented 2 years ago

Quicktype seems usable from code: https://github.com/quicktype/quicktype#calling-quicktype-from-javascript

That could be a good starting point. I'm wondering how it will handle arrays, though. Some of them should be tuples instead of arrays so we might have to keep an eye on that.

thmsndk commented 2 years ago

That does seem interesting, only glanced over it quickly, would we be using it more than once initially?

I still think we need to handcraft some of the types and modify the generated types.

Telokis commented 2 years ago

I'm always trying to think about the moment where an update will make our types incorrect, that's why I'm thinking of automating some things.

What do you think we'll need to handcraft? When it comes to G, I think we mainly need to add documentation to the fields to explain what they represent and/or what they're used for but I'm probably overlooking some things.

My reasoning was that we have the G object entirely, its type is 100% available and we don't need to guess anything (like we have to for some functions, sadly... :'( ). That's why I instantly thought of static analysis and automation.
I was even thinking of a way for us to specify a configuration that our generator would take as input to generate the types according to our groups and filters. And that configuration would also allow us to tell the generator to attach documentation automatically.

For example:

{
  "items": {
    groupBy: "type",
    docs: {
      "g": "Base gold cost of the item",
      "s": "Maximum stack size"
    }
  }
}

Of course this is just an idea and maybe I'm making things too complicated for no good reason. Please do let me know your thoughts on all that!

Telokis commented 2 years ago

I've just made a first draft of a very basic TypeScript generation for G.items. It's available on this branch: https://github.com/adventureland-community/typed-adventureland/blob/feature/generated-items-types/generate-data-types.ts

Telokis commented 1 year ago

@thmsndk Can we consider this issue to be done? We can create more precise ones if we later find wrong/missing things.

earthiverse commented 1 year ago

@thmsndk Can we consider this issue to be done? We can create more precise ones if we later find wrong/missing things.

I'd like to see G.drops implemented before you mark this done!

thmsndk commented 1 year ago

I think we should create separate issues for the following based on the fact that they are disabled in the generate configs