New strong-type EventDispatcher and Threejs strong-types events

[rafaelsc]

BREAKING CHANGE

• Old strong-typing EventDispatcher is not compatible with this new typing. Requiring migration.

Why

• This PR is to improve the EventDispatcher typing solving the EventDispatcher Typing is very limited. #368

What

• Update EventDispatcher Typing
• Update EventDispatcher Typing testing
• Add strong-types events to current known ThreeJS events

Checklist

• Checked the target branch (current goes master, next goes dev)
• Added myself to contributors table
• Ready to be merged

[0b5vr]

I'm not too expertised to see if it's conventionally a good solution.
I see that the motivation is valid at least.

[joshuaellis]

So just in a "eli5" way, you're passing what you expect the target to be now? e.g. like event handlers in react?

[rafaelsc]

ELI5:

The Goal is Events in ThreeJs can be strongly typed and allow any other external lib or user of the three-types extends with his own custom strong events.

Non ELI5:

The goal is for THREE.EventDispatcher to have the same typing capabilities as any DOM HTML Element strong typing events from typescript dom.generated.d.ts. This link has a good explanation of how DOM String event works in Typescript, and how we can extend them. https://43081j.com/2020/11/typed-events-in-typescript

Looking at the Typing of HTMLElement the strong typed Events are mapped via HTMLElementEventMap with a hierarchy of EventMap interfaces, and this Map's the addEventListener can expose the correct strong typed event.
This PR will add these same features to THREE.EventDispatcher.

Other changes are:

• Type-checking when dispatching known events.
• Add EventMap with strong-typed events for all current know events fired by ThreeJS.
• Allow dispatch and Listener of unknowing custom events. (Current not possible with current typing).
• Remove any from the fired ev.target to the correct type from the Dispatcher.
• Add a Disposable interface to use in any Class that has the dispose() method.

[joshuaellis]

Nice explanation, lovely addition.

[Methuselah96]

What is the purpose of these export statements?

[rafaelsc]

Because the ArcballControlsEventMap is not exported.
DefinitelyTyped rules complain that there is a non-exported type.
This is the way to tell the DefinitelyTyped rule that you want not to export something by default.
This is used in many places to allow not export the EventMap

[Methuselah96]

Looks good from a first glance, will take a closer look soon.

[Methuselah96]

Left some initial thoughts.

[Methuselah96]

Wouldn't TTarget be more accurate than TSource?

[rafaelsc]

Update with your suggestion.

[Methuselah96]

How about keeping this called EventListener and keeping the order of the generics the same as before to avoid the change in this type being a breaking change?

[rafaelsc]

Update with your suggestion.

[Methuselah96]

So this supports dispatching basically any event if the key does not exist in the event map. Are there common use-cases where this is necessary? Seems like it might be nice to have the ability to block any undeclared events.

[rafaelsc]

EventDispatcher allow dispatch and listen for any event.
In my opinion, Typescript types should not block or reduce the features/capabilities of the Javascript code, only add helpful type information to reduce the errors caused by not using the correct typing.
Blocking unknown event types would reduce the features/capabilities of the original EventDispatcher code.

I don't know how common is for people to use custom events on THREE.Object3D or custom EventDispatcher . but I imagine that some are. And I think EventDispatcher typing should allow them to decide if they want the custom event to be strong-typed or not.

I can imagine that I would prefer to create a simple quick and dirty custom Object3D or other EventDispatcher sub-type and test each feature without wanting to have to do the bureaucracy of creating custom strong-type events for a test.

[Methuselah96]

I think it's reasonable to want to block unknown event types if the user knows exactly what set of event types they want to allow.

It seems like we could support both approaches. For the places where we want the event type to be open, the event map could include an index signature. For the places where we want the event type to be closed, the event map could not include an index signature.

My problem with the current approach is that it makes it impossible to block unknown events, whereas relying on whether there's an index signature or not would allow for either approach.

Thoughts?

[Methuselah96]

I suppose there's no way to make a closed hierarchy of events in the way DOM events are typed either. Let me reconsider this.

[Methuselah96]

It looks we're no longer producing an error in this case since @ts-expect-error was changed to ts-expect-error. Is that intentional?

[rafaelsc]

Sorry, that was an error. Tests are now updated and with better comments.

[rafaelsc]

Sorry, I finally fixed the conflict.

[Methuselah96]

@rafaelsc I'm interested in completing this, but I don't want to step on your toes. Let me know if you plan to get back this, otherwise I will probably take this on.

[Methuselah96]

To follow-up on the discussion regarding whether to allow dispatching unknown events.

After trying a few approaches, taking a more strict approach to events and allowing the user to loosen the typings seems like the best option.

Rationale

The main reason for taking this approach is that it is the only safe approach. Consider a simplified XRHandSpace definition:

interface Object3DEventMap {
    added: {};
    removed: {};
}

interface WebXRSpaceEventMap extends Object3DEventMap {
    select: { data: XRInputSource };
}

class XRHandSpace extends Group<WebXRSpaceEventMap> {}

If we allowed dispatching unknown events, it would allow dispatching a bad select event if the XRHandSpace is considered as an Object3D like this:

const handSpace = new THREE.XRHandSpace();

function dispatchSelectEvent(object: THREE.Object3D) {
    object.dispatchEvent({ type: 'select' });
}

dispatchSelectEvent(handSpace);

This breaks how inheritance and types should work and it seems best to at least provide the option of avoiding this issue.

Dispatching unknown events

Because of that, we only allow dispatching defined events. There are a few ways to add events to the defined event map.

1. If you're just creating an Object3D, then you can define the event map in the generic:

const allowSpecificEvent = new THREE.Object3D<THREE.Object3DEventMap & { test: { data: number } }>();
allowSpecificEvent.dispatchEvent({ type: 'test', data: 5 });

const allowAnyEvent = new THREE.Object3D<THREE.Object3DEventMap & Record<string, { [key: string]: unknown }>>();
allowAnyEvent.dispatchEvent({ type: 'any', data: 5 });

2. If you're creating a new class, you can define it in the class definition:

class AllowSpecificEvent extends Object3D<THREE.Object3DEventMap & { test: { data: number } }> {}
const allowSpecificEventClass = new AllowSpecificEvent();
allowSpecificEventClass.dispatchEvent({ type: 'test', data: 5 });

class AllowAnyEvent extends Object3D<THREE.Object3DEventMap & Record<string, { [key: string]: unknown }>> {}
const allowAnyEventClass = new AllowAnyEvent();
allowAnyEventClass.dispatchEvent({ type: 'any', data: 5 });

3. If you just want to allow to dispatch any event for any Object3D you could augment the Object3DEventMap interface:

declare module 'three' {
    interface Object3DEventMap {
        [key: string]: { [key: string]: unknown };
    }
}

const allowAnyEvent = new THREE.Object3D();
allowAnyEvent.dispatchEvent({ type: 'any', data: 5 });

Conclusion

I'm hoping this change provides more good than harm, please let me know if this ends up causing a lot of churn and we can take a look at what we can do to make it better.