Conversation
6eb8826 to
2acffd3
Compare
Co-authored-by: ckohen <chaikohen@gmail.com>
`emit()` on internal emitters accepts any input, so these aren't testing anything at all, as evidenced by the fact that a lot of these are passing supposedly invalid input without errors.
2acffd3 to
68d642c
Compare
| * @since v0.1.26 | ||
| */ | ||
| class EventEmitter<T extends EventMap<T> = DefaultEventMap> { | ||
| class EventEmitter<T extends EventMap<T> = any> { |
There was a problem hiding this comment.
Note: This now has to be any rather than {} due to the miracle that is exactOptionalPropertyTypes. Interassignability between class instances has to match the type parameters, and with exOPT enabled, the compiler will complain that the properties provided in the event map are "missing" from NoEventMap when trying to assign an EventEmitter<T> to EventEmitter.
To be fair, any does rather accurately capture the behaviour of the default EventEmitter when it comes to event types.
There was a problem hiding this comment.
I am glad EOPT caught this, but, this feels very odd; do things actually still work when any??
There was a problem hiding this comment.
They do, in that both {} and any pass the constraint of a type that is assignable to Record<keyof T, any[]> but which also satisfies the conditional {} extends T, which is the gatekeeper for EE using the conventional rather than the event-mapped signatures.
| declare const any: any; | ||
|
|
||
| { | ||
| using abortListener = events.addAbortListener(new AbortController().signal, (event) => { |
There was a problem hiding this comment.
As an aside, I do believe that this is the first explicit resource management test in DT... 😄
What could go wrong..?
POC for two sets of EventEmitter changes for v25.
The first (4ebdaa4) is a general cleanup of the events module that I've been meaning to get round to for a while. This also contains alterations to the event map behaviour of EventEmitter, mostly along the lines of #71060, with some minor tweaks. This is working well – the only breakage in the DT tests is that EventEmitter no longer plays with
Functiontypes, which was a deliberate choice (but can always be revisited).Aside from some serious simplification, the main advantage of the event map changes is that EE now permits events not in the event map and assumes them to have a default
(...args: any[]) => voidlistener type, which plays a lot better inheritance-wise.The second (7f09a01) is a new paradigm for internal event emitters. These can't use
EventEmitter<...>event maps, as deep assignability in dark corners of the ecosystem relies on these methods resolving to a permissive signature accepting anystring | symboland(...args: any[]) => voidcombo. However, we can still do better than the current approach, which is to define a hodge-podge of EventEmitter overrides with some event-specific overloads on each and every EventEmitter in the API. The old approach has many issues:addListener,onetc.) but whether others likeoff,removeListeneretc. were also overridden was essentially random.k * n, and some of these blocks were 100+ lines long.The new approach is an InternalEventEmitter pattern, which provides event-mapped type-hinting behaviour while still maintaining the same permissive behaviour as before. This should be very much non-breaking, and indeed caused no complaints in the DT tests.
Events for internal emitters are now truly hierarchical: a derived emitter D can have a DEventMap that inherits from CEventMap -> BEventMap -> etc., ensuring consistency and deduplication.