Type issues with `emit` and `emitWithAck`

[ZachHaber]

Describe the bug
When using emit, emitWithAck, and the server side variants, there are issues with the types of Broadcaster, Server, Namespace, and Socket types.

To Reproduce

Socket.IO server version: 4.7.2

Server

import { Server } from "socket.io";

export interface ServerToClientEvents {
  basicEmit: (a: number, b: string) => void;
  withAck: (d: string, callback: (e: number) => void) => void;
  // multiple Ack arguments doesn't work on broadcasts - single target
  withAck2: (d: string, callback: (e: number, f: string) => void) => void;
}
export interface ClientToServerEvents {}
const io = new Server<ClientToServerEvents,ServerToClientEvents>(3000, {});

//Emits
// no callback
io.timeout(1).emit('basicEmit',3,'')
// Likely should be a type error as it doesn't have a callback at all
// Currently just times out...

// One arg in callback
io.emit('withAck', 'hello', (...args) => { });
//=> ❌ typeof args === [number], should be [number[]]
io.timeout(1).emit('withAck', 'hello', (...args) => {});
//=> ✅ typeof args === [Error, number[]]
io.to('1').to('2').emit('withAck','',(...args)=>{})
//=> ❌ typeof args === [number], should be [number[]]

// Two args in callback
io.emit('withAck2', 'hello', (...args) => { });
//=> ❌ typeof args === [number,string], should be [number[]]
io.timeout(1).emit('withAck2','hello',(...args)=>{});
//=> ❌ typeof args === [Error,number,string], should be [Error,number[]]

// EmitWithAcks
// no callback
io.emitWithAck('basicEmit',3,'')
//=> type error: Expected 2 arguments, got 3 (the last argument is clipped off)
// Should just be a type-error, as without `timeout` applied, it will likely just never resolve
// if you apply a timeout, then it just times out eventually due to not having a callback

// One arg in callback
io.emitWithAck('withAck','hello');
//=>❌ type is Promise<any>, should be Promise<number[]>
io.timeout(1).emitWithAck('withAck','hello')
//=>✅ type is Promise<number[]>

// Two args in callback - doesn't even work in timeout broadcaster
io.emitWithAck('withAck2','hello');
//=>❌ type is Promise<any>, should be Promise<number[]>
io.timeout(1).emitWithAck('withAck2','hello')
//=>❌ type is Promise<any>, should be Promise<number[]>


io.on("connection", (socket) => {
  // emit is fine on the socket instance itself
  socket.broadcast.emit('withAck','hello',(...args)=>{});
  //=>❌ typeof args === [number], should be [number[]]
  socket.broadcast.timeout(1000).emit('withAck', 'hello',(...args)=>{});
  //=>✅ typeof args === [Error, number[]]
  // important to note that whatever types exist needs to work with socket.broadcast.timeout(1) AND socket.timeout(1).broadcast
  socket.timeout(1).broadcast.timeout(1).emit('withAck','hello',(...args)=>{});
  //=>❌ typeof args === [Error, Error, number[]], should be [Error, number[]]
  // Needs to not re-decorate the event if an error was already added. (not sure what benefit allowing this gives in the first place
  
  // emitWithAck is backwards here vs the broadcast based ones...
  socket.emitWithAck('withAck','hello');
  //=>✅ Promise<number>
  socket.timeout(1).emitWithAck('withAck','hello');
  //=>❌ Promise<any>, should be Promise<number>
  
  // two args
  socket.emitWithAck('withAck2','hello');
  //=>❌ Promise<any>, should be Promise<number>
  socket.timeout(1).emitWithAck('withAck2','hello');
  //=>❌ Promise<any>, should be Promise<number>
});

Socket.IO client version: x.y.z

Client
I haven't checked on the client, but I assume the same types are used on here too (to some extent)

Expected behavior
Type definitions should be consistent with what socket.io is doing so that we can fully rely on them. As of the moment, they are wrong in a few places that are either frustrating (any instead of a real type) or problematic (number instead of number[]).

Platform:

• Typescript 5.2

Additional context
I have some first passes at solving some of these issues that I can work towards improving and turning into a PR, if that's desired.

[darrachequesne]

Hi! Thanks for this detailed analysis 👍

I think most of the issues come from the fact that calling .timeout() is (at least currently) mandatory on the server side, since there is no default timeout value (the client has an ackTimeout option though).

So, for example:

// INVALID (will time out directly anyway)
io.emit('withAck', 'hello', (...args) => { });
// VALID
io.timeout(1).emit('withAck', 'hello', (...args) => { });

Also, broadcasting with an acknowledgement from each client is only meant to be used with one callback argument. This was to prevent surprising behaviors such as:

export interface ServerToClientEvents {
  // VALID
  withAck: (d: string, callback: (e: number) => void) => void;
  // INVALID
  withAck2: (d: string, callback: (e: number, f: string) => void) => void;
}

io.timeout(5000).emit("withAck", "hello", (err, responses) => {
  // responses: number[]
  // responses[0]: number
});

io.timeout(5000).emit("withAck2", "hello", (err, responses) => {
  // responses: [number, string][]
  // responses[0]: [number, string]
});

Does that make sense?

[ZachHaber]

The main reason why I put up this issue and the accompanying pull request is that the Typescript Types on this library don't reflect the reality of the library, which makes them somewhere between dangerous (when a type exists, but is completely wrong) to pointless (when a type comes back as any).

I think most of the issues come from the fact that calling .timeout() is (at least currently) mandatory on the server side, since there is no default timeout value (the client has an ackTimeout option though).

I had incorrectly assumed that emitting to multiple clients without timeout would just never timeout, thanks for the clarification. I completely missed the warning alert on the docs page for Server.emit

Here are some of my thoughts on how to align the type defintions with the reality of the library based on the current behaviors (beyond what I currently have in my PR):

A fair amount of this is based on my understanding that it is currently impossible to add a timeout without creating an instance of the BroadcastOperator class.

• Server & Namespace:

  • Make emit [Type only] skip the callback function entirely so users won't get confused and assume it will work.
  • Remove the emitWithAck [Type and implementation] function as it's dangerously unusable directly on these classes without a timeout set as the promise currently never fulfills.

• BroadcastOperator:

  • emit [Type change only] should similarly skip the callback function until there's an Error as the first arg, which chaining timeout adds in.
  • emitWithAck [Type change only] shouldn't accept any events without Error as the first argument to the callback, and it definitely shouldn't accept any events without a callback function defined.

[darrachequesne]

@ZachHaber that sounds great 👍