Skip to content
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

socket listen with AbortSignal #1592

Open
ZachHaber opened this issue Oct 13, 2023 · 2 comments
Open

socket listen with AbortSignal #1592

ZachHaber opened this issue Oct 13, 2023 · 2 comments
Labels
enhancement New feature or request

Comments

@ZachHaber
Copy link

ZachHaber commented Oct 13, 2023

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
/**
 * @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<typeof socket.on>[0]} Ev Name of the event
 * @param {Ev} ev Name of the event
 * @param {Parameters<typeof socket.on<Ev>>[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
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<Ev extends Parameters<typeof socket.on>[0]>(
  ev: Ev,
  listener: Parameters<typeof socket.on<Ev>>[1],
  { signal, once }: SocketOnOptions = {}
) {
  if (once) {
    socket.once(ev, listener);
  } else {
    socket.on(ev, listener);
  }
  signal?.addEventListener(
    "abort",
    () => {
      socket.off(ev, listener);
    },
    { once: true }
  );
}
@ZachHaber ZachHaber added the enhancement New feature or request label Oct 13, 2023
@darrachequesne
Copy link
Member

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
Copy link
Author

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 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants