about single parameter listener error, (.apply is not a function error)

[salihguru]

Describe the bug Socketio on function requests event name as 1st parameter and callback as 2nd parameter. For this, a check is done under the hood, but it is not enough. We need to improve the control a little more. Because if the callback is not sent, we get the following error.

To Reproduce

Please fill the following code example:

Socket.IO server version: 4.4.1

Server

// it doesn't matter what

Socket.IO client version: 4.4.1

Client

import { io } from "socket.io-client";

const socket = io("ws://localhost:3000/", {});

socket.on("connect", () => {
  console.log(`connect ${socket.id}`);

  socket.on("mybestevent.v1") // causes an error.
});

socket.on("disconnect", () => {
  console.log("disconnect");
});

Expected behavior The code under the hood is as follows:

I think we can update the code here as follows. We even issue a warning like in the else block, telling the user that he should send a callback. I think it would be a better use.

Emitter.prototype.emit = function(event) {
   this._callbacks = this._callbacks || {};
   
   var args = new Array(arguments.length - 1), callbacks = this._callbacks['$' + event];
  
   for (var i = 1; i < arguments.length; i++) {
      args[i - 1] = arguments[i];
   }

   if(callbacks) {
     callbacks = callbacks.slice(0);
     for (var i = 0, len = callbacks.length; i < len; i++) {
         if(!!callbacks[i] && typeof callbacks[i] === 'function') {
              callbacks[i].apply(this, args);
         }else {
              console.warn("Looks like you forgot to add a callback to a listener. Please check your listeners.")
         }
     }
   }
}

Platform:

• Device: any
• OS: any

Additional context I would be glad if you consider it.

[darrachequesne]

Hi! I guess we could add a check here:

Emitter.prototype.on =
Emitter.prototype.addEventListener = function(event, fn){
  if (typeof fn !== "function") {
    throw new Error("please provide a function");
  }
  this._callbacks = this._callbacks || {};
  (this._callbacks['$' + event] = this._callbacks['$' + event] || [])
    .push(fn);
  return this;
};

What do you think?

That being said, we could also add a warning if there is no matching callback here:

Emitter.prototype.emit = function(event){
  this._callbacks = this._callbacks || {};

  var args = new Array(arguments.length - 1)
    , callbacks = this._callbacks['$' + event];

  for (var i = 1; i < arguments.length; i++) {
    args[i - 1] = arguments[i];
  }

  if (callbacks) {
    callbacks = callbacks.slice(0);
    for (var i = 0, len = callbacks.length; i < len; ++i) {
      callbacks[i].apply(this, args);
    }
  } else {
    console.warn(`no callback registered for ${event}`);
  }

  return this;
};

(note: we'd have to remove these console.warn in production)

[salihguru]

Thank you for your callback. It looks so much better this way!

In terms of production, there is no warning given in most of the codes. We can skip it for now to adapt to the project. But I think we should definitely show a warning based on an env value to be determined by the user. Because finding this solution required extra focus.