Foo-Foo-MQ / foo-foo-mq

Abstractions around RabbitMQ
MIT License
49 stars 24 forks source link

TypeScript: should `emit` and `off` be a part of `Broker` namespace? #44

Closed GoFightNguyen closed 1 year ago

GoFightNguyen commented 1 year ago

foo-foo-mq version: 8.0.0 typescript version: 4.4.4

We have unit tests asserting that our code correctly handles events raised by foo-foo-mq. here is an example

import * as sinon from 'sinon';
import * as rabbot from 'foo-foo-mq';
import { assert } from 'chai';
import { EventEmitter } from 'events';

describe('#registerEvents specific events checks', () => {
    let sandbox: sinon.SinonSandbox;
    let eventEmitter: EventEmitter;

    beforeEach(() => {
      sandbox = sinon.createSandbox();
      eventEmitter = new EventEmitter();

       // our code that responsds to foo-foo-mq events by re-emitting
      registerEvents(eventEmitter, 1, 5);
    });

    afterEach(() => {
      rabbot.off();
    });

    after(() => {
      sandbox.restore();
    });

    it('expected data returned from the failed event', (done) => {
      const failureData = {
        consecutiveFailures: 1,
      };
      const timeStampObj = new Date();

      eventEmitter.on('failed', (data) => {
        assert.deepEqual(data, {
          connectionAttempts: failureData.consecutiveFailures,
          attemptsRemaining: 4,
          remainingTimeToRetry: 1, // how to calculated value
          message:
            'Attempting to reconnect until time runs out or retries are expired',
        });
        done();
      });
      rabbot.emit('failed', failureData, timeStampObj);
    });
});

The problem is that tsc fails with error TS2339: Property 'emit' does not exist on type 'typeof Broker'.

Should emit be added to the Broker type/namespace? Or is there a better way to simulate a foo-foo-mq event?

GoFightNguyen commented 1 year ago

In addition, we are experiencing the same tsc error for off(), which is used in the afterEach()

zlintz commented 1 year ago

Yes you are correct. You can see those in the monologue.js file. The current index.d.ts was graciously provided by another user but it likely was they were not using that functionality. Feel free to make a PR for this and we can get it merged in. There is another change that also needs to get release soon so now is a good time for this to get in @GoFightNguyen

GoFightNguyen commented 1 year ago

For history, monologue.js is in reference to node-monologue

GoFightNguyen commented 1 year ago

@zlintz Any idea when/if a release will be made soon?

zlintz commented 1 year ago

Thanks for checking, I just released foo-foo-mq@8.0.1 for you