0

I'm looking for a good pattern to follow that combines Javascript classes, event listeners, and async programming.

I'm trying to write a shared library for my project that handles connections to a Kafka server. The kafka-node library requires that you connect and then set a listener for a 'connected' event. I would like to mask that complexity for other libraries that need to produce messages to kafka.

The following code essentially works, but I feel that it's complex (resolving the Promise within an event listener) and there must be a better way

class KafkaProducer {

   constructor(id) {

      this.connected_mode = process.env.KAFKA_CONNECT !== 'false';
      this.hostname = process.env.NODE_ENV === 'development' ? '192.168.99.100:2181' : 'service-kafka:2181';
      this.id = id ? id : 'no_name_specified';

      log.info(`KafkaProducer: hostname: ${this.hostname} id: ${this.id}`);

      if (this.connected_mode === false) {
        log.info('KafkaProducer: Running in disconnected mode');
        this.ready = true;
        return;
      }

      this.client = new Client(this.hostname, this.id, {
         sessionTimeout: 300,
         spinDelay: 100,
         retries: 2
      });
      this.ready = false;
      this.connected = false;
      this.producer = false;
      this.metrics = {};
   }

   initialize() {

      let that = this;

      log.silly(`KafkaProducer.initialize: Initializing with connected_mode=${this.connected_mode}`);

      return new Promise((resolve, reject) => {

         if (that.connected_mode === false) {
            log.silly('KafkaProducer.initialize: Returning early in disconnected mode');
            return resolve();
         }

         if (that.ready && that.producer) {
            log.silly('KafkaProducer.initialize: Returning early as both ready and producer are set');
            return resolve();
         }

         log.silly(`KafkaProducer.initialize: Creating Producer with client details connectionString=${that.client.connectionString}`)
         that.producer = new Producer(that.client, {requireAcks: 1});

         that.producer.on('ready', () => {
            log.info('KafkaProducer: Producer is ready');
            that.ready = true;
            that.connected = true;
            return resolve();
         });

         that.producer.on('error', (err) => {
            log.error(`KafkaProducer: ${err}`);
         });
      });
   }

Other code that needs to send a Kafka message can import this class, check the ready attribute and initialize if necessary

KafkaProducer.initialize().then(() => {
    KafkaProducer.sendMessage('foo');
});

But resolving the Promise from within an event listener smells like bad code. Is there a better way?

Edit: I mistakenly wrote "return" the promise rather than "resolve"... Have corrected the question

SimonMorris
  • 127
  • 1
  • 10
  • 1
    "*check the ready attribute and initialize if necessary*" - no, they should simply *always* call the `initialise` method. If it already is initialised, it won't do any harm. – Bergi Mar 08 '18 at 12:45
  • Where are you returning a promise inside of an event listener? Did you mean "resolving"? – Bergi Mar 08 '18 at 12:45
  • 1
    You are not actually returning but resolving the issue from within the event listener which I would consider just fine – newBee Mar 08 '18 at 12:47
  • What is `KafkaProducer`, an instance or the constructor? Where is your class instantiated? Have a [look at this](https://stackoverflow.com/a/24686979/1048572) for asynchronous initialisation of an instance. – Bergi Mar 08 '18 at 12:47
  • 1
    You probably should use `this.producer.once` instead of `.on`, and also not forget to call `reject()` in the case of an error, but otherwise this looks fine. – Bergi Mar 08 '18 at 12:48
  • @Bergi indeed, my implementation always calls initialise. My class is truncated in my question above, but it's exported as a singleton into my project. – SimonMorris Mar 08 '18 at 19:30
  • @Bergi Thank you for your final comment. Changing to ```this.producer.once``` is a good idea. Happy that it doesn't look like an incorrect pattern to you. – SimonMorris Mar 08 '18 at 19:31
  • @SimonMorris You should not use `class` syntax for singletons. It would probably be more appropriate to export a promise (or an `initalise` function returning that promise if you need it lazy) that fulfills with the plain `KafkaProducer` object, written as an object literal with methods. – Bergi Mar 08 '18 at 20:37
  • @Bergi - could you explain the downside of using a Class for a singleton. Is it just better to do so, or there is a definite difference? Appreciate your help – SimonMorris Mar 09 '18 at 20:44
  • 1
    @SimonMorris Classes and prototypes should only be used when you have multiple instances that share some common things via inheritance. It's better to keep your code simple and not use features that you don't need. (Also, a singleton constructed from a `class` is not a real singleton, everyone could make a second instance from it) – Bergi Mar 10 '18 at 18:25

0 Answers0