search

three-types / three-ts-types

TS type library for the popular webgl library threejs
233 stars 152 forks source link

EventDispatcher typing consistency #1143

Closed s-rigaud closed 2 weeks ago

s-rigaud commented 1 month ago

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

https://github.com/three-types/three-ts-types/blob/c8cea806df55a23ff020819be3abb86c27a4adfe/types/three/src/core/EventDispatcher.d.ts#L84


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

https://github.com/three-types/three-ts-types/blob/c8cea806df55a23ff020819be3abb86c27a4adfe/types/three/src/core/EventDispatcher.d.ts#L52-L55

https://github.com/three-types/three-ts-types/blob/c8cea806df55a23ff020819be3abb86c27a4adfe/types/three/src/core/EventDispatcher.d.ts#L56


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 commented 1 month ago

Good question! Those may have been left in by mistake in https://github.com/three-types/three-ts-types/pull/369. I'm experimenting with removing them in https://github.com/three-types/three-ts-types/pull/1145, but it does seem to break some tests, so I need to look into that some more.

Methuselah96 commented 1 month ago

~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.