League-of-Foundry-Developers / foundry-vtt-types

Unofficial type declarations for the Foundry Virtual Tabletop API
MIT License
112 stars 54 forks source link

Improve actor.itemTypes typing #1551

Open Socolin opened 2 years ago

Socolin commented 2 years ago

Hello,

I don't know if typescript allow it but could it be possible to automatically types items of actor.itemTypes:

When I'm accessing to types like:

let weapons = itemsByType['weapon'];
// Then I want to access to the data without additional check:
weapon[0].data.data.<property available weapon>

If I create a globa.d.ts with the following

import {ConfiguredDocumentClass} from '@league-of-foundry-developers/foundry-vtt-types/src/types/helperTypes';

declare global {
    class Actor extends ClientDocumentMixin(foundry.documents.BaseActor) {
        get itemTypes(): Record<
            'weapon',
            Array<InstanceType<ConfiguredDocumentClass<typeof foundry.documents.BaseItem>> & {data: {type: 'weapon'}}>
        >;
    }
}

it seems to work but I'm getting an error on class Actor TS2300: Duplicate identifier 'Actor'

1- Why am I getting an error class Actor could you provide an example type merging with more complex case in: https://github.com/League-of-Foundry-Developers/foundry-vtt-types/wiki/A-Quick-Guide-to-Declaration-Merging ? 2- Is there a way to make it available for all types without having to define each overload manually ?

ghost91- commented 2 years ago

I haven't checked every detail yet but we could potentially make this work by using

    get itemTypes(): {
      [Key in foundry.documents.BaseItem['data']['type']]: Array<
        InstanceType<ConfiguredDocumentClass<typeof foundry.documents.BaseItem>> & {
          data: foundry.data.ItemData & { type: Key; _source: ItemDataSource & { type: Key } };
        }
      >;
    };

The above makes your example work, but I am not sure if there are any negative implications of that. We will need to check.

Regarding your questions:

  1. You can't declare classes multiple times, you need to use an interface to use declaration merging here. However, that still won't work because successive declarations of the same property need to have the same type.
  2. See above.
ghost91- commented 2 years ago

Tests are still passing, which sounds promising. Any opinions / comments on this @kmoschcau @FloRad?

ghost91- commented 2 years ago

Unfortunately, this change breaks other stuff, see #1588, so we are going to revert it for now, until we find a solution.

Socolin commented 2 years ago

Just and idea, maybe there could be some kind of config to enable this, like for game with let game: 'game' extends keyof LenientGlobalVariableTypes ? Game : Game | {};

ghost91- commented 2 years ago

Just and idea, maybe there could be some kind of config to enable this, like for game with let game: 'game' extends keyof LenientGlobalVariableTypes ? Game : Game | {};

I don't think that's a practical solution. I agree that it's good to make stuff configurable for common situations but so far, this seems more like a niche thing, so I don't think it's worth the added complexity.

I'll just leave this issue open for now in case we find a solution, but we won't work actively on this.