Export typescript types for events with details

[mpharoah]

Updated PR description:
This pull request exports Typescript types for all Shoelace events and makes the internal emit function type safe to ensure all emitted events have the correct shape for their detail property.

Event types can be imported either individually as a default export from each event type file, or in bulk from the events.ts file which re-exports all events in a single file.

This removes the need for consumers using TypeScript to redefine the events, and ensures that any breaking changes in Shoelace events will be detected at transpile time by consumers.

All events except for sl-error have a unique (or non-existent) detail property; however sl-error is the exception since it has detail of shape { status: number } when emitted from the sl-include component, but has no detail when emitted from other components.

An unused event handler in sl-dropdown was also removed as per this comment.

Original PR description:
This merge request exports types for Shoelace events that have event details. Types are not exported for events without any details since CustomEvent or just Event can be used for those events.

This removes the need for consumers using TypeScript to redefine the events, and ensures that any breaking changes in Shoelace events will be detected at transpile time by consumers.

I didn't add a definition for sl-error here because it has no details in some contexts (eg. sl-icon), but does have details when fired from sl-include, so I wasn't sure what you wanted done for this case. All other events with details should be covered.

[vercel]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated
shoelace	✅ Ready (Inspect)	Visit Preview	💬 Add your feedback	Feb 15, 2023 at 2:42PM (UTC)

[claviska]

Thank you! This is something people have been asking for, so I really appreciate the PR.

At a glance, this looks good. My only concern is since not all events have details, it's not clear to contributors and maintainers when an event should be added. I wonder if adding all events with null or undefined payloads for consistency would help with that. What do you think?

Aside — I'd like to hold off on merging this until #1167 is in and #1180 is addressed. I'm hoping both will be finished up this week. Hopefully that won't create too many conflicts.

[mpharoah]

Sure, that sounds good. All these types will just be aliases for CustomEvent or CustomEvent<null> though. Do you think it would be good to continue to put each event in its own file, or should all events with no details just be in their own file?

[claviska]

Do you think it would be good to continue to put each event in its own file, or should all events with no details just be in their own file?

I'm OK with putting them all in separate files OR putting all events (with and without details) in one file. The idea is that things are consistent and easier to maintain, even if on the surface it feels a bit inefficient. 😄

[xdev1]

[Edit] Oh, haven't read the two previous comments, when I've written the comment below, anyway...

---

@mpharoah @claviska Very happy to here that someone finally adds those SL event types 😃👍
Please allow me to give my two cents (it's just about increasing DX):

• There's really no doubt that it would increase DX if ALL Shoelace custom events have a dedicated type

• I personally would prefer event file names to end with -event.ts, this makes things a bit easier when Ctrl+P shows you a file list... [Edit] Maybe one single file for events like suggested above might really be an easier solution

• Would also be great to register those SL custom event types, to make the SL event type available for addEventListener etc. Frankly, I'm not really sure what's to 100% correct solution for this, but I'm always using:

    declare global {
      interface HTMLElementEventMap {
        'my-event': MyEvent
      }
    }

[mpharoah]

I looked into that, but it didn't really seem to work. Adding the explicit addEventListener implementations for each event doesn't affect compilation-- it only affects code hints, and unfortunately, at least in VS Code, it doesn't really work as expected. Defining it for EventTarget doesn't cause it to be shown as a code hint for any of the other classes derived from EventTarget such as Element, HTMLElement, Document, etc.

I think this is because of the way it is implemented in typescript. The addEventListener specializations are implemented using keyof <some type for an event map>, but augmenting that event map doesn't work because the keyof has already been evaluated before your extra event types were added, so it doesn't work.

[mpharoah]

Do you think it would be good to continue to put each event in its own file, or should all events with no details just be in their own file?

I'm OK with putting them all in separate files OR putting all events (with and without details) in one file. The idea is that things are consistent and easier to maintain, even if on the surface it feels a bit inefficient. smile

Sounds good. I'll continue with my approach of doing both (one file for each, plus an events.ts file that re-exports all of them from a single file).

[xdev1]

I looked into that, but it didn't really seem to work.

Actually it works:
Please see TS playground example

[claviska]

I didn't add a definition for sl-error here because it has no details in some contexts (eg. sl-icon), but does have details when fired from sl-include, so I wasn't sure what you wanted done for this case.

Sorry, I forgot to reply to this. We should identify all events that don't emit a consistent details payload and either make them consistent or, when it doesn't make sense, mark the payload as optional.

[mpharoah]

I looked into that, but it didn't really seem to work.

Actually it works: Please see TS playground example

Probably a VS Code issue then. Or using keyof on type that is later updated is just undefined behaviour. Nevermind, it works in VS Code- it was just being finicky.

Looks like to get it everywhere (except EventTarget which never gets any hints) you need to add it to ElementEventMap and GlobalEventHandlersEventMap

It also doesn't update addEventListener on type Node or EventTarget either. It has to be Element, Document, Window, or something inheriting from them. But none of the standard DOM events get hints on Node or EventTarget either, so that's fine.

Actually, just adding it to GlobalEventHandlersEventMap only should be fine. Since the only hints for type Element (not HTMLElement which also inherits the gloabl events) are fullscreenchange and fullscreenerror and not any other standard events. So I don't think we need to add it to that.

[mpharoah]

I think I'm going to switch to a single file because it will allow making emit be type-safe much easier. Any preference on where that file should go? Just the root src directory? Or just keep the event folder and only have one file in it?

[claviska]

Can src/declaration.d.ts be moved into a new types folder without any issue?

src/types/declaration.d.ts
src/types/events.ts

This will give us a place to add future type files. If not, throwing it in src is fine.

[mpharoah]

Nevermind, combining them into one file was not necessary.

[mpharoah]

Found an unused and undocumented event sl-activate here: https://github.com/shoelace-style/shoelace/blob/next/src/components/dropdown/dropdown.ts#L345

It's bound to an event handler, but it looks like nothing ever fires it.

[claviska]

Found an unused and undocumented event sl-activate here: https://github.com/shoelace-style/shoelace/blob/next/src/components/dropdown/dropdown.ts#L345

It's bound to an event handler, but it looks like nothing ever fires it.

Whoops. That was removed in 2.0.0-beta.22. It's unused, so feel free to get rid of it.

[xdev1]

@mpharoah Matt, there's a new event type now in the next branch. It's called sl-invalid and does not have a detail value. Would you mind adding it to your PR whenever you find time to do so?

[mpharoah]

Changed null details to an empty object to be consistent with all other events in Shoelace

[xdev1]

Thanks a lot 👍
May I ask: What's the advantage of using
type SlInvalidEvent = CustomEvent<Record<PropertyKey, never>>; instead of just using type SlInvalidEvent = CustomEvent<{}>; (see here)?

[mpharoah]

Suprisingly, {} doesn't actually mean empty object when used as a type in Typescript. I actually initially used CustomEvent<{}> but then got a linter error about it not doing what you expect:

error Don't use {} as a type. {} actually means "any non-nullish value".

• If you want a type meaning "any object", you probably want object instead.
• If you want a type meaning "any value", you probably want unknown instead.
• If you want a type meaning "empty object", you probably want Record<string, never> instead @typescript-eslint/ban-types

[mpharoah]

Added some comments to make it more clear what the Typescript type manipulation stuff is doing. The PR is now ready for review again.

[claviska]

Thank you for this PR. You are a TypeScript ninja! I'll admit that these types scare me a bit, but your use of comments to describe what each piece is doing is superb and makes it a much easier pill to swallow. 😅