[Live] Add signature overload for on and off methods of component

[YummYume]

Q	A
Bug fix?	no
New feature?	no
Issues
License	MIT

Following some feedback in #1676. This PR only affects TypeScript typing.

Provides automatic typing for the on and off events of a component, depending on the event's name. Also changes the return type allowed from an event listener to allow passing an asynchronous function. This is a pretty common thing to do (the need to use await), but TypeScript/linters will often yell at you for doing so if the event listener does not expect a Promise to be returned.

(also fixed a typing issue with NodeJS.Timer being used instead of number)

[smnandre]

Thank you for that @YummYume !

Could you rebuild the dist file ? It feels you updated those files "manually" (i may be wrong on this, TS is not my strongest force 😅 )

From the root of the monorepo, this should be enough i think

npm run build

(This is why the CI is red Symfony UX / Check for UnBuilt JS Dist Files (pull_request) Failing after 1m)

[YummYume]

This is weird 🤔 I didn't touch the dist files I simply ran the yarn build command. And somehow the Autocomplete controller needed to be rebuilt. Now it's fine.

No idea why the LiveComponent tests aren't passing, though, they're all green.

[smnandre]

Related to dependencies / Symfony 7.1.. and don't bother with Autocomplete if you're not working on it ... or you won't ever end this PR haha

[WebMamba]

I am not sure about this fix. setInterval is native function well supported why use the one from window?

[YummYume]

Using setInterval or window.setInterval results in the same thing in a browser context. However, when using setInterval, TypeScript was mistakenly thinking that this was the Nodejs setInterval function, which doesn't return a number like in a browser, which was causing a typing error warning during builds.

[WebMamba]

Ok thanks for your answer!

[WebMamba]

Instead of having all of those definitions why not just use type ComponentHookName?
on(hookName: ComponentHookName, callback: (component: Component) => MaybePromise): void;

[YummYume]

Because the callbacks receive different arguments depending on the event. By explicitly overloading this method, TypeScript (and your IDE) now knows which events are available, and the arguments that they each receive. It's a much nicer developer experience, and it should work for any IDE even when not using TypeScript.

[squrious]

What do you think about adding callback types too? Like ConnectHook or ConnectCallback

[squrious]

Just a suggestion, I'm not so comfortable with TypeScript, but what would you think about something like that?

type Hooks = {
    'connect': (component: Component) => MaybePromise,
    'disconnect': (component: Component) => MaybePromise,
    'request:started': (requestConfig: any) => MaybePromise
    'render:finished': (component: Component) => MaybePromise
    'response:error': (backendResponse: BackendResponse, controls: {displayError: boolean}) => MaybePromise
    'loading.state.started': (element: HTMLElement, request: BackendRequest) => MaybePromise
    'loading.state.finished': (element: HTMLElement) => MaybePromise
    'model:set': (model: string, value: any, component: Component) => MaybePromise,
}

type ComponentHookName = keyof Hooks;

type HookCallback<T extends string> = T extends ComponentHookName
    ? Hooks[T]
    : (...args: any[]) => MaybePromise;

class Component {
    on<T extends ComponentHookName|string>(name: T, callback: HookCallback<T>): void {
        this.hooks.register(hookName, callback);
    }
    off<T extends ComponentHookName|string>(name: T, callback: HookCallback<T>): void {
        this.hooks.unregister(hookName, callback);
    }
}

[YummYume]

Hi @squrious ! I tested this approach, and after a few tweaks, it gives the same result as the current implementation, and is a bit easier to use for maintaining many events, rather than overloading the methods manually. I personally wouldn't be opposed to it. 😄

[squrious]

Nice 😄

[YummYume]

So I went ahead and implemented your idea with the conditional, it's way more maintainable and achieves the same thing. 👍

[WebMamba]

Sounds good to me! Thanks a lot @YummYume!

[WebMamba]

Ok thanks for your answer!

[kbond]

Awesome first contribution @YummYume, thank you!