openhab / openhab-js

openHAB JavaScript Library for JavaScript Scripting Automation
https://www.openhab.org/addons/automation/jsscripting/
Eclipse Public License 2.0
38 stars 31 forks source link

Type declarations should not inline type definitions #392

Open wertzui opened 3 weeks ago

wertzui commented 3 weeks ago

After upgrading to the latest version, VS code tells me that there is a problem when I pass an Item to triggers.ItemStateChangeTrigger, because Item does not expose "__#5@#getToggleState"(): "PAUSE" | "PLAY" | "OPEN" | "CLOSED" | "ON" | "OFF" which is defined here https://github.com/openhab/openhab-js/blob/7f2b7a41a5d68e0464a05dc9654799ffedd35abf/types/triggers.d.ts#L36 and got introduced with https://github.com/openhab/openhab-js/commit/2348409e0a6af49c133b0438578b4f8c6423b3d1.

Besides this method definition being obviously wrong (It should probably be getToggleState(): "PAUSE" | "PLAY" | "OPEN" | "CLOSED" | "ON" | "OFF"), I think triggers.d.ts should not expose its own Item definition, but rather use the one from items.d.ts.

florian-h05 commented 2 weeks ago

394 fixes the issue with "__#5@#getToggleState"(): "PAUSE" | "PLAY" | "OPEN" | "CLOSED" | "ON" | "OFF" by not making this method private anymore.

I think triggers.d.ts should not expose its own Item definition, but rather use the one from items.d.ts.

Yes, but our type definitions are emitted by the TypeScript compiler, which currently inlines type definitions. This is an upstream issue: https://github.com/microsoft/TypeScript/issues/37151

Dopeyr commented 2 weeks ago

This might be solvable with Typescript 5.5, ref: https://github.com/microsoft/TypeScript/issues/22160

Adding

/**
 * @import { Item } from './items/items'
 */

to triggers.js no longer adds the export of the Item type, so vscode does not suggest Item from triggers.js.

However, the resulting type def in in types/triggers.d.ts is:

/**
 * Creates a trigger that fires upon an Item changing state.
 *
 * @example
 * ItemStateChangeTrigger('my_item'); // changed
 * ItemStateChangeTrigger('my_item', 'OFF', 'ON'); // changed from OFF to ON
 * ItemStateChangeTrigger('my_item', undefined, 'ON'); // changed to ON
 * ItemStateChangeTrigger('my_item', 'OFF', undefined); // changed from OFF
 *
 * @memberof triggers
 * @param {Item|string} itemOrName the {@link Item} or the name of the Item to monitor for change
 * @param {string} [oldState] the previous state of the Item
 * @param {string} [newState] the new state of the Item
 * @param {string} [triggerName] the optional name of the trigger to create
 */
export function ItemStateChangeTrigger(itemOrName: {
    new (rawItem: HostItem): {
        rawItem: HostItem;
        persistence: import("./items/item-persistence");
        semantics: import("./items/item-semantics");
        readonly type: string;
        readonly name: string;
        readonly label: string;
        readonly state: string;
        readonly numericState: number;
        readonly quantityState: import("./quantity").Quantity;
        readonly rawState: HostState;
        readonly members: any[];
        readonly descendents: any[];
        readonly isUninitialized: boolean;
        getMetadata(namespace?: string): {
            namespace: ItemMetadata;
        } | ItemMetadata | null;
        replaceMetadata(namespace: string, value: string, configuration?: object): {
            configuration: any;
            value: string;
        } | null;
        removeMetadata(namespace?: string): ItemMetadata | null;
        sendCommand(value: string | number | JSJoda.ZonedDateTime | Quantity | HostState): void;
        sendCommandIfDifferent(value: string | number | JSJoda.ZonedDateTime | Quantity | HostState): boolean;
        sendIncreaseCommand(value: number | Quantity | HostState): boolean;
        sendDecreaseCommand(value: number | Quantity | HostState): boolean;
        getToggleState(): "PAUSE" | "PLAY" | "OPEN" | "CLOSED" | "ON" | "OFF";
        sendToggleCommand(): void;
        postToggleUpdate(): void;
        postUpdate(value: string | number | JSJoda.ZonedDateTime | Quantity | HostState): void;
        readonly groupNames: string[];
        addGroups(...groupNamesOrItems: any[]): void;
        removeGroups(...groupNamesOrItems: any[]): void;
        readonly tags: string[];
        addTags(...tagNames: string[]): void;
        removeTags(...tagNames: string[]): void;
        toString(): any;
    };
} | string, oldState?: string, newState?: string, triggerName?: string): HostTrigger;

Which looks a bit odd...

florian-h05 commented 2 weeks ago

I played around with TS 5.5 and was not able to solve the issue by using @import ...

Dopeyr commented 2 weeks ago

I think it solves one issue with the autosuggest, and introduces another.

Currently Item is suggested from multiple files (note it includes triggers as it's exported there)

image

After TS 5.5 and adding @import to triggers.js, it no longer suggests Item from triggers

image

But when autosuggesting the parameters for ItemStateChangeTrigger (for example) the auto complete is horrible

image

florian-h05 commented 2 weeks ago

Ultimately I think we won’t have a solution here as long as https://github.com/microsoft/TypeScript/issues/37151 is open.