socketio / socket.io

Realtime application framework (Node.JS server)
https://socket.io
MIT License
61.16k stars 10.11k forks source link

socket listen with AbortSignal #5119

Open ZachHaber opened 1 year ago

ZachHaber commented 1 year ago

Is your feature request related to a problem? Please describe. When setting up an listener, you end up having to define your callback in a separate variable in order to be able to clear the listener. This makes it so that you lose the convenient typing that the socket.on<Ev> generic provides.

Describe the solution you'd like I'd like an AbortSignal to be able to be passed into the socket.on function via a third options argument. The options can mirror the DOM api's AddEventListenerOptions:

{
  once?: boolean;
  signal?: AbortSignal;
}

When the signal fires, the listener would be removed. This will make it much more convenient to cleanup the listeners, and could potentially allow for cleaning up multiple listeners with a single controller call. This also aligns with the trend towards adopting AbortControllers in various APIs (axios ,fetch ,DOM eventListeners, node's timer promises, and likely more).

Describe alternatives you've considered It isn't complicated to set up a wrapper function to accomplish this from a user side. So maybe adding something like this to the documentation could suffice?

JavaScript Variant ```js /** * @typedef SocketOnOptions * @property {AbortSignal} [signal] If an AbortSignal is passed for signal, then the event listener will be removed when signal is aborted. * @property {boolean} [once] When set to true, once indicates that the callback will only be invoked once after which the event listener will be removed. */ /** * Adds the listener function as an event listener for ev. * * This allows for convenience of passing options including a * {@link SocketOnOptions.signal `signal`} to remove the listener when desired * * @template {Parameters[0]} Ev Name of the event * @param {Ev} ev Name of the event * @param {Parameters>[1]} listener Callback function * @param {SocketOnOptions} [param2] */ export function socketOn( ev, listener, { signal, once } = {} ) { if (once) { socket.once(ev, listener); } else { socket.on(ev, listener); } signal?.addEventListener( "abort", () => { socket.off(ev, listener); }, { once: true } ); } ```
TypeScript Variant ```ts export interface SocketOnOptions { /** * If an AbortSignal is passed for signal, then the event listener will be removed when signal is aborted. */ signal?: AbortSignal; /** * When set to true, once indicates that the callback will only be invoked once after which the event listener will be removed. */ once?: boolean; } /** * Adds the listener function as an event listener for ev. * * This allows for convenience of passing options including a * {@link SocketOnOptions.signal `signal`} to remove the listener when desired * * @param ev Name of the event * @param listener Callback function * @param options */ export function socketOn[0]>( ev: Ev, listener: Parameters>[1], { signal, once }: SocketOnOptions = {} ) { if (once) { socket.once(ev, listener); } else { socket.on(ev, listener); } signal?.addEventListener( "abort", () => { socket.off(ev, listener); }, { once: true } ); } ```
darrachequesne commented 1 year ago

If I understand correctly, you want to be able to do something like this:

const controller = new AbortController();
const signal = controller.signal;

socket.on("foo", () => { ... }, { signal });
socket.on("bar", () => { ... }, { signal });

// and then later
controller.abort();

Is that right?

However, I think that the philosophy has always been to follow the Node.js EventEmitter interface, which does not currently allow a 3rd argument to EventEmitter.on() (note: AbortSignal is used in the events package, but for another operation).

So I think this should rather go to the documentation. What do you think?

ZachHaber commented 1 year ago

That is precisely what I want to be able to do, if you are following the Node.js EventEmitter interface for the client, then it makes sense to continue to do so.

Then adding this to the documentation should suffice until Node.js adds signals to the EventEmitter interface 😄