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

EventDispatcher typing consistency #1143

Closed
s-rigaud opened this issue Aug 12, 2024 · 2 comments · Fixed by #1145
Closed

EventDispatcher typing consistency #1143

s-rigaud opened this issue Aug 12, 2024 · 2 comments · Fixed by #1145

Comments

@s-rigaud
Copy link
Contributor

The EventDispatcher only has one method definition for dispatchEvent which is strict.

dispatchEvent<T extends Extract<keyof TEventMap, string>>(event: BaseEvent<T> & TEventMap[T]): void;



While there are multiple method definitions for addEventListener which make the typing a bit loose.

addEventListener<T extends Extract<keyof TEventMap, string>>(
type: T,
listener: EventListener<TEventMap[T], T, this>,
): void;

addEventListener<T extends string>(type: T, listener: EventListener<{}, T, this>): void;



I don't understand why addEventListener is loose when it directly depends over dispatchEvent which is strict. Shouldn't the loosest implementation based over regular string be removed ? It would raise error when listening to none defined event and thus be more type safe 🤔
It will be especially useful when creating custom controls inheriting from EventDispatcher.

@Methuselah96
Copy link
Contributor

Methuselah96 commented Aug 12, 2024

Good question! Those may have been left in by mistake in #369. I'm experimenting with removing them in #1145, but it does seem to break some tests, so I need to look into that some more.

@Methuselah96
Copy link
Contributor

Methuselah96 commented Aug 12, 2024

I was able to fix all the tests fairly easily, and they seemed like positive changes. 🎉 Let's give this a go!

Blocked by an error in another package's tests, will need to fix that first.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants