Typing event emitters #764

unao · 2022-02-07T15:40:22Z

This PR adds proper typing to event emitters for all classes documented in the official docs: https://mediasoup.org/documentation/v3/mediasoup/api/.

Listening to (and emitting) events is fully type checked.

The PR is compatible with current way of registering internal events (names starting with @).

Initially I wanted only to augment types, but finally very minor changes to EnhancedEventEmitter were introduced.

One mismatch between code and docs discovered is argument to ActiveSpeakerObserver.on('dominantspeaker'). According to docs it is Producer according to code { producer: Producer }.

jmillan

Nice!

Added some comments

jmillan · 2022-02-08T09:03:33Z

node/src/DataProducer.ts

 const logger = new Logger('DataProducer');

-export class DataProducer extends EnhancedEventEmitter
+export class DataProducer extends EnhancedEventEmitter<{ transportclose: [] }>


No Events type constructed and used here?

Agree: consistency is better than succinctness.

jmillan · 2022-02-08T09:09:09Z

node/src/Router.ts

 const logger = new Logger('Router');

-export class Router extends EnhancedEventEmitter
+export class Router extends EnhancedEventEmitter<{ workerclose: [] }>


Events type for consistency?

jmillan · 2022-02-08T09:11:42Z

node/src/Worker.ts

@@ -176,7 +178,7 @@ const workerBin = process.env.MEDIASOUP_WORKER_BIN
 const logger = new Logger('Worker');
 const workerLogger = new Logger('Worker');

-export class Worker extends EnhancedEventEmitter
+export class Worker extends EnhancedEventEmitter<{ died: [Error]}>


Same, defining Events makes it more consistent and allows for seamlessly adding new events in the future.

node/src/EnhancedEventEmitter.ts

ibc · 2022-02-08T09:22:00Z

node/src/Consumer.ts

+	trace: [ConsumerTraceEventData];
+}
+
+type ConsumerEvents = Pick<ObserverEvents, 'score' | 'layerschange' | 'trace'>


Even if Consumer and Consumer.observer events sometimes match, can we repeat their definition instead of picking some of them from the other?

…vents; add Producer events

ibc

This is a great PR. I've requested some changes to get more consistency.

ibc · 2022-02-10T10:34:50Z

node/src/ActiveSpeakerObserver.ts

+type ObserverEvents = RtpObserverEvents & {
+	dominantspeaker: [{ producer: Producer }];
+}
+
+type Events = {
+	routerclose: [];
+	dominantspeaker: [{ producer: Producer }];
+}


Some concerns here that affects all files and not just this one:

Can we please define first class events and then observer events?.

routerclose is not just an event in ActiveSpeakerObserver class. It's an event that all RtpObserver inherited classes.

The name of RtpObserver class in unfortunate since it also contains an observer. So events of the observer of RtpObserver should be named RtpObserverObserverEvents.

And events of the RtpObserver parent class should be named RtpObserverEvents.

Some classes (such as Transport and RtpObserver export they XxxxxxEvents and XxxxxObserverEvents. However, in child classes this PR defines type Events and type ObserverEvents. For consistency we should export these as well and prefix their name with the class name.

Not sure why ESLint doesn't complain but we mandate opening brackets to be in the next line.

So IMHO this should be as follows:

Suggested change

type ObserverEvents = RtpObserverEvents & {

dominantspeaker: [{ producer: Producer }];

}

type Events = {

routerclose: [];

dominantspeaker: [{ producer: Producer }];

}

type ActiveSpeakerEvents = RtpObserverEvents &

{

dominantspeaker: [{ producer: Producer }];

}

type ActiveSpeakerObserverEvents = RtpObserverObserverEvents &

{

dominantspeaker: [{ producer: Producer }];

}

ibc · 2022-02-10T10:43:36Z

node/src/RtpObserver.ts

+export type RtpObserverEvents = {
+	close: [];
+	pause: [];
+	resume: [];
+	addproducer: [Producer];
+	removeproducer: [Producer];
+}
+
+type Events = {
+	routerclose: [];
+}


Suggested change

export type RtpObserverEvents = {

close: [];

pause: [];

resume: [];

addproducer: [Producer];

removeproducer: [Producer];

}

type Events = {

routerclose: [];

}

export type RtpObserverEvents =

{

routerclose: [];

}

export type RtpObserverObserverEvents =

{

close: [];

pause: [];

resume: [];

addproducer: [Producer];

removeproducer: [Producer];

}

ibc · 2022-02-10T10:44:29Z

node/src/Transport.ts

+type ObserverEvents = {
+	close: [];
+	newproducer: [Producer];
+	newconsumer: [Consumer];
+	newdataproducer: [DataProducer];
+	newdataconsumer: [DataConsumer];
+	trace: [TransportTraceEventData];
+
+}


Suggested change

type ObserverEvents = {

close: [];

newproducer: [Producer];

newconsumer: [Consumer];

newdataproducer: [DataProducer];

newdataconsumer: [DataConsumer];

trace: [TransportTraceEventData];

}

export type TransportObserverEvents =

{

close: [];

newproducer: [Producer];

newconsumer: [Consumer];

newdataproducer: [DataProducer];

newdataconsumer: [DataConsumer];

trace: [TransportTraceEventData];

}

ibc · 2022-02-10T10:45:34Z

node/src/Transport.ts

@@ -81,9 +81,23 @@ export interface TransportTraceEventData

 export type SctpState = 'new' | 'connecting' | 'connected' | 'failed' | 'closed';

+export type TransportBaseEvents = { routerclose: []; trace: [TransportTraceEventData] }


No need for "base" in the name:

Suggested change

export type TransportBaseEvents = { routerclose: []; trace: [TransportTraceEventData] }

export type TransportEvents = { routerclose: []; trace: [TransportTraceEventData] }

…types

unao · 2022-02-10T17:08:40Z

@ibc
Updated the PR accordingly. I introduced bit more verbosity for the sake of decoupling.

Not sure why ESLint doesn't complain but we mandate opening brackets to be in the next line.

According to their docs; you should have following rule to enable it for typescript:

{
  // note you must disable the base rule as it can report incorrect errors
  "brace-style": "off",
  "@typescript-eslint/brace-style": ["error"]
}

Unfortunately (which I checked) it does not add support for type; only for (...) enum, interface, namespace and module declarations.

unao · 2022-02-10T17:31:06Z

I opened an issue for fixing the brace-style rule

ibc · 2022-02-10T19:18:57Z

I'm running CI checks on this PR. It looks good. I'll do my final review tomorrow and merge if no more changes are requested.

Thanks for this!

ibc

Fantastic. Merging, THANKS

ibc · 2022-02-22T10:19:27Z

Released in 3.9.7

unao added 8 commits February 4, 2022 17:55

EnhancedEventEmitter - improve types for listeners

2224d23

Worker, Router: typed listeners

b8a69b4

Transports - typed listeners

f0b9db1

Producer - typed listeners

4d32baf

Consumer - typed listeners

733e895

DataConsumer, DataProducer - typed listeners

a82705c

RtpObserver, ActiveSpeakerObserver, AudioLevelObserver - typed listeners

8726b1d

typed listeners - regenerate lib

eaf9671

jmillan reviewed Feb 8, 2022

View reviewed changes

node/src/EnhancedEventEmitter.ts Show resolved Hide resolved

ibc reviewed Feb 8, 2022

View reviewed changes

unao added 2 commits February 8, 2022 12:33

typed listenders - stronger demarcation between events and observer e…

19f233a

…vents; add Producer events

expose prependListener and prependOnceListener in EnhancedEventEmitter

c569de9

unao requested a review from ibc February 9, 2022 13:25

ibc requested changes Feb 10, 2022

View reviewed changes

typed listener: consitent naming and exports; stricter decoupling of …

6a4b0f9

…types

unao requested a review from ibc February 10, 2022 17:09

jmillan approved these changes Feb 17, 2022

View reviewed changes

ibc approved these changes Feb 22, 2022

View reviewed changes

ibc merged commit fe5b3ab into versatica:v3 Feb 22, 2022

unao deleted the typing-event-emitters branch February 22, 2022 12:08

unao mentioned this pull request Feb 23, 2022

Improve typings for event emitters versatica/mediasoup-client#200

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Typing event emitters #764

Typing event emitters #764

unao commented Feb 7, 2022

jmillan left a comment

jmillan Feb 8, 2022

unao Feb 8, 2022

jmillan Feb 8, 2022

jmillan Feb 8, 2022

unao Feb 8, 2022

ibc Feb 8, 2022

unao Feb 8, 2022

ibc left a comment

ibc Feb 10, 2022

ibc Feb 10, 2022

ibc Feb 10, 2022

ibc Feb 10, 2022

unao commented Feb 10, 2022

unao commented Feb 10, 2022

ibc commented Feb 10, 2022

ibc left a comment

ibc commented Feb 22, 2022

		@@ -81,9 +81,23 @@ export interface TransportTraceEventData

		export type SctpState = 'new' \| 'connecting' \| 'connected' \| 'failed' \| 'closed';

		export type TransportBaseEvents = { routerclose: []; trace: [TransportTraceEventData] }

	export type TransportBaseEvents = { routerclose: []; trace: [TransportTraceEventData] }
	export type TransportEvents = { routerclose: []; trace: [TransportTraceEventData] }

Typing event emitters #764

Typing event emitters #764

Conversation

unao commented Feb 7, 2022

jmillan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ibc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

unao commented Feb 10, 2022

unao commented Feb 10, 2022

ibc commented Feb 10, 2022

ibc left a comment

Choose a reason for hiding this comment

ibc commented Feb 22, 2022