Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Type declarations should not inline type definitions #392

Open
wertzui opened this issue Oct 28, 2024 · 5 comments
Open

Type declarations should not inline type definitions #392

wertzui opened this issue Oct 28, 2024 · 5 comments
Labels
external bug Something isn't working and it isn't our fault

Comments

@wertzui
Copy link

wertzui commented Oct 28, 2024

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

"__#6@#getToggleState"(): "PAUSE" | "PLAY" | "OPEN" | "CLOSED" | "ON" | "OFF";
and got introduced with 2348409.

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 added a commit to florian-h05/openhab-js that referenced this issue Nov 3, 2024
Private methods are not supported by .d.ts type declarations, so do not use them!

Refs openhab#392.

Signed-off-by: Florian Hotze <[email protected]>
@florian-h05
Copy link
Contributor

#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: microsoft/TypeScript#37151

@florian-h05 florian-h05 added the external bug Something isn't working and it isn't our fault label Nov 3, 2024
@florian-h05 florian-h05 changed the title types/triggers.d.ts should not expose its own Item definition Type declarations should not inline type definitions Nov 3, 2024
florian-h05 added a commit that referenced this issue Nov 3, 2024
Private methods are not supported by .d.ts type declarations, so do not
use them!

Refs #392.

Signed-off-by: Florian Hotze <[email protected]>
@Dopeyr
Copy link
Contributor

Dopeyr commented Nov 7, 2024

This might be solvable with Typescript 5.5, ref: microsoft/TypeScript#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
Copy link
Contributor

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

@Dopeyr
Copy link
Contributor

Dopeyr commented Nov 7, 2024

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
Copy link
Contributor

Ultimately I think we won’t have a solution here as long as microsoft/TypeScript#37151 is open.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
external bug Something isn't working and it isn't our fault
Projects
None yet
Development

No branches or pull requests

3 participants