-
Notifications
You must be signed in to change notification settings - Fork 10.1k
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
Add strictly typed events #3822
Conversation
lib/index.ts
Outdated
@@ -645,14 +698,28 @@ export class Server extends EventEmitter { | |||
return this; | |||
} | |||
|
|||
public on< |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suppose we would also need to strongly type once
, removeListener
, etc. Perhaps it might make sense to introduce an abstract class StrictEventEmitter
between EventEmitter
and Server
; this would just override all methods to do all the strict typing, and call super
's implementations. What do you think of that approach?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about adding once()
too? I think it should cover most use cases. And we'll add StrictEventEmitter
in the future, if some users request it. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I ended up moving it all to StrictEventEmitter
to avoid duplicating all the overrides of on
and once
. And I added once
, which has the exact same signature.
|
||
describe("server", () => { | ||
describe("no event map", () => { | ||
describe("on", () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wrote this test in the same style as Mocha unit tests (describe
/ it
), but these are not actually run. Perhaps it could make sense to refactor to some other structure, which might be less confusing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The current format looks fine in my opinion. Maybe with a comment at the top stating that this file is covered by tsd
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea! Done :)
a66b88a
to
9f9bdd5
Compare
9f9bdd5
to
1e2f9cf
Compare
@MaximeKjaer I see you've already rebased your PR against the latest changes, nice! 👍 I'll take a look at it as soon as possible, but it looks good at first sight 👌 |
f51c5b8
to
86df1b6
Compare
lib/index.ts
Outdated
@@ -645,14 +698,28 @@ export class Server extends EventEmitter { | |||
return this; | |||
} | |||
|
|||
public on< |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about adding once()
too? I think it should cover most use cases. And we'll add StrictEventEmitter
in the future, if some users request it. What do you think?
.github/workflows/ci.yml
Outdated
@@ -24,3 +24,4 @@ jobs: | |||
- run: npm test | |||
env: | |||
CI: true | |||
- run: npm run test:types |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you think about adding npm run test:types
in the npm test
command instead? (to prevent surprising CI failures while npm test
runs fine locally)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perfect! I placed it under npm test
. I split that command into two subcommands, just to make it a little more readable.
|
||
describe("server", () => { | ||
describe("no event map", () => { | ||
describe("on", () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The current format looks fine in my opinion. Maybe with a comment at the top stating that this file is covered by tsd
?
lib/index.ts
Outdated
@@ -156,8 +156,58 @@ interface ServerOptions extends EngineAttachOptions { | |||
connectTimeout: number; | |||
} | |||
|
|||
export class Server extends EventEmitter { | |||
public readonly sockets: Namespace; | |||
/** |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about moving those declarations in their own file? (completely cosmetic, it's your call 👼 )
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm all for it! With the addition of StrictEventEmitter
I think it made a lot of sense to move it all to a separate file.
removeEventListener does not exist in component-emitter
@@ -952,8 +955,7 @@ describe("socket.io", () => { | |||
const sio = new Server(srv); | |||
srv.listen(() => { | |||
const clientSocket = client(srv, { reconnection: false }); | |||
clientSocket.on("connect", function init() { | |||
clientSocket.removeListener("connect", init); | |||
clientSocket.once("connect", () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Improving the typing of client Socket
s showed that clientSocket.removeListener
does not exist in component-emitter, so this had to be fixed in order to get tests to compile.
Syntax: ```ts interface ClientToServerEvents { "my-event": (a: number, b: string, c: number[]) => void; } interface ServerToClientEvents { hello: (message: string) => void; } const io = new Server<ClientToServerEvents, ServerToClientEvents>(httpServer); io.emit("hello", "world"); io.on("connection", (socket) => { socket.on("my-event", (a, b, c) => { // ... }); socket.emit("hello", "again"); }); ``` The events are not typed by default (inferred as any), so this change is backward compatible. Note: we could also have reused the method here ([1]) to add types to the EventEmitter, instead of creating a StrictEventEmitter class. Related: #3742 [1]: https://github.com/binier/tiny-typed-emitter
Merged as 0107510. I made a few minor updates:
Hope that's OK for you 👼 Anyway, awesome work, thanks a lot ❤️ |
Hey @MaximeKjaer! I'm the author of typed-socket.io, which I wrote a few years ago and seems pretty similar to this functionality you've now integrated here. I've added a note to the readme that similar functionality is now supported in socket.io itself, but since I'm not a huge socket.io user anymore, I'm not sure what the exact comparison would be. My library is still used by a fair amount of people, so if you have any specific info you think would be useful to my readme, let me know. |
Syntax: ```ts interface ClientToServerEvents { "my-event": (a: number, b: string, c: number[]) => void; } interface ServerToClientEvents { hello: (message: string) => void; } const io = new Server<ClientToServerEvents, ServerToClientEvents>(httpServer); io.emit("hello", "world"); io.on("connection", (socket) => { socket.on("my-event", (a, b, c) => { // ... }); socket.emit("hello", "again"); }); ``` The events are not typed by default (inferred as any), so this change is backward compatible. Note: we could also have reused the method here ([1]) to add types to the EventEmitter, instead of creating a StrictEventEmitter class. Related: socketio#3742 [1]: https://github.com/binier/tiny-typed-emitter
The kind of change this PR does introduce
Current behavior
User events
EventEmitter
functions, namelyon
andemit
, accept a string as the event name, andany[]
as the remaining parameters. This allows users to:Reserved events
Because the event emitter functions aren't strictly typed, it also means that
server.on("connection", (socket: Socket) => { ... })
needs an explicit cast ofsocket: Socket
.New behavior
User events
TypeScript users can define interfaces for the API between server and client by defining interfaces:
When creating the
Server
, users can optionally pass this interface as a type parameter:With this in place, calls to
Socket.emit
andSocket.on
are strictly typed to only allow emitting and listening to"hello"
events:It's also possible to define different messages for each direction of the connection:
Note that if no type parameter is passed to
Server
, then events are typed as before.Reserved events
server.on("connection", socket => { ... })
now correctly infers theSocket
type forsocket
. No cast needed! Other reserved events are now also correctly inferred. Strict typing of reserved events is also present when no type parameters are passed toServer
.This does make this a breaking change if users rely on the
on
callback arguments being of typeany
. This was the case in this project's tests, wheresocket
being of typedany
was used to access private members ofSocket
. The fix is to castas any
, which makes the implicitany
type explicit.Other information (e.g. related issues)
Fixes #3742