search

apollographql / graphql-subscriptions

:newspaper: A small module that implements GraphQL subscriptions for Node.js
MIT License
1.59k stars 133 forks source link

Refactor async event emitter #185

Closed robbywashere-zz closed 2 years ago

robbywashere-zz commented 5 years ago

This PR greatly reduces the complexity of src/event-emitter-to-async-iterator but relies on changes to src/test/test.js and could use additional input on how to handle the unsubscription of events on the given event emitter. return() should otherwise unsubscribe the listener but does not in this PR.

grantwwu commented 5 years ago

@robbywashere Any reason why this was closed? Just curious...

robbywashere-zz commented 5 years ago

@grantwwu Thanks for the interest. It didn't pass the build tests, and also I made a mistake in that it only worked with single events. This is sort of the gist of what I have so far. This does work with multiple events, but that setImmediate(rs) is a bit ugly. Any thoughts?

Also : iterator.return() is not currently removing the registered listener(s)

const { EventEmitter } = require('events');

async function* streamify(
  emitter,
  events
) {
  const eventsArray = [].concat(events);
  let q = [];
  for (let event of eventsArray) emitter.on(event,arg => q.push(arg));
  while (true) {
    await new Promise(rs=>setImmediate(rs));
    while (q.length) {
      yield q.shift();
    }
  }
}
hwillson commented 2 years ago

The codebase has changed quite a bit since this PR was opened, so this isn't something we can accept at this time. Thanks very much for the suggestion!