ChatTriggers / ctjs

An in-progress rewrite of the ChatTriggers mod for Fabric 1.20
MIT License
21 stars 8 forks source link

Please update the typings #126

Closed Omega172 closed 3 months ago

Omega172 commented 3 months ago

When registering a 'chat' trigger the callback requested is a (...args: unknown[]) => void through some testing I came to the conclusion that the only argument ever provided to the callback is a ChatTrigger$Event I suggest that the requested call back should be a (event: ChatTrigger$Event) => void

What this would mean registering a new 'chat' trigger would look like this

register('chat', (Event: com...ChatTrigger$Event) => { 
    // Do stuff
});

I have also noticed that the typings that were generated and provided with the beta 12 release are incorrect. Example:

// typings.d.ts Line 42022
const ChatTrigger$Event: {
    message: com.chattriggers.ctjs.api.message.TextComponent;
    new(message: com.chattriggers.ctjs.api.message.TextComponent): com.chattriggers.ctjs.api.triggers.ChatTrigger$Event;
}
interface ChatTrigger$Event extends com.chattriggers.ctjs.api.triggers.CancellableEvent { 
}

// Should be
interface ChatTrigger$Event extends com.chattriggers.ctjs.api.triggers.CancellableEvent { 
    message: com.chattriggers.ctjs.api.message.TextComponent;
    new(message: com.chattriggers.ctjs.api.message.TextComponent): com.chattriggers.ctjs.api.triggers.ChatTrigger$Event;
}

Now this is not the only instance of incorrect typings but it is the main one the bothered me. Because when working in a TypeScript environment (which is able to be properly loaded by ChatTriggers) I can not get proper auto complete or type hints and get constant errors because types do not match or do not have to properties they should,

Examples: image

register('chat', (Event) => { 
    cancel(Event);
})

image

register('chat', (Event) => {
    const Message = Event.message.getUnformattedText();
    const Tokens = Message.split(' ');

    console.log(Tokens[5]);
}).setChatCriteria('${*}').setStart().setFormatted(false);
camnwalter commented 3 months ago

Messages with captured parameters in the criteria also pass in those parameters before the chat event. For instance

register('chat', (name, message, event) => {
}).setChatCriteria('${name}: ${message}');

Also, yes there are some issues with the typings so we can still tweak them, but we separate interfaces and consts as a way to mimic static and instance properties/methods.

Omega172 commented 3 months ago

Ok so only if the message captures something like the player name will it be passed through, thanks that clears that up

But in that case the callback def should be (event: ChatTrigger$Event, ...args: (string | unknown)[]) => void

Omega172 commented 3 months ago

Also, yes there are some issues with the typings so we can still tweak them, but we separate interfaces and consts as a way to mimic static and instance properties/methods.

I'll have to admit I have never seen that before, but it is def causing issues with auto complete and compiling the TS to JS

mattco98 commented 3 months ago

But in that case the callback def should be (event: ChatTrigger$Event, ...args: (string | unknown)[]) => void

The event is always the last parameter, not the first. The signature would have to be (...args: (string | unknown)[], event: ChatTrigger$Event) => void, but unfortunately Typescript does not support vararg parameters in the non-final position in a parameter list.

I'll have to admit I have never seen that before, but it is def causing issues with auto complete and compiling the TS to JS

The typings shouldn't have any effect on compilation, it's only for the IDE. They've always been full of errors since the API surface is so large, and the Java type system doesn't map perfectly to Typescript.

Omega172 commented 3 months ago

I had noticed a little while after making that first reply, that the event was always the last argument so we can ignore that comment,

when it comes to the Java types, when attempting to compile to TypeScript to JavaScript using tsc from the npm typescript package it does cause some errors because types do not have the correct properties and thus it fails, and if attempting to load any typescript file directly with cjs there are errors because type specifiers and other small things are not supported.

When it comes to errors with Java types from Java.type() I believe it is acceptable to have the user manually define the type and its properties to clear any errors.

But as I showed in the initial post of this issue there are some types that were just not defined properly and did not have any properties defined, I did take the time to understand the interface and const separation that is used and I actually like it, it was just the first time I have ever seen it. I did notice several places where the interface and const were defined properly like Client for example, I would just like to see this issue fixed, so I could get proper property hints and auto-complete.

mattco98 commented 3 months ago

when it comes to the Java types, when attempting to compile to TypeScript to JavaScript using tsc from the npm typescript package it does cause some errors because types do not have the correct properties and thus it fails, and if attempting to load any typescript file directly with cjs there are errors because type specifiers and other small things are not supported.

Then I'm pretty sure you have it set up incorrectly, as this is the first time I'm hearing of this issue. I'd suggest asking in our discord for more help there.

But as I showed in the initial post of this issue there are some types that were just not defined properly and did not have any properties defined,

The only issue you showed in the initial post is that the event parameter of a chat trigger was not typed correctly, which I just said is not possible to fix. For cancel, you'd have to do cancel(event as CancellableEvent). Unless you have some more examples to provide, there is nothing actionable in this issue. I want to reemphasize though that the typings will never be perfect, and there will always be an error somewhere. The goal is to get them to be pretty good.

Omega172 commented 3 months ago

The only issue you showed in the initial post is that the event parameter of a chat trigger was not typed correctly, which I just said is not possible to fix. For cancel, you'd have to do cancel(event as CancellableEvent). Unless you have some more examples to provide, there is nothing actionable in this issue. I want to reemphasize though that the typings will never be perfect, and there will always be an error somewhere. The goal is to get them to be pretty good.

ChatTrigger$Event was not defined properly

Omega172 commented 3 months ago

Because my questions and concerns have been answered I will close this issue