Introduce internal API call queuing mechanism #43

2018-03-21T12:42:15Z

laurynas-karvelis commented

2018-03-21 12:42:15 +00:00

(Migrated from github.com)

Introduce internal API call queuing mechanism to send messages sequentially (no more random order)

laurynas-karvelis commented

2018-03-21 12:50:12 +00:00

(Migrated from github.com)

Thank you for the merge of my previous PR and super quick turnaround :).
This PR will be probably the most wanted feature from this module for my BE app :). Queuing! 👏

Thank you for the merge of my previous PR and super quick turnaround :). This PR will be probably the most wanted feature from this module for my BE app :). Queuing! :clap:

👍 1

mdibaiee (Migrated from github.com) requested changes 2018-03-21 14:35:36 +00:00

mdibaiee (Migrated from github.com) left a comment

Thanks! This is definitely a welcome improvement, there is a single detail I've commented below

src/functions/api.js

mdibaiee (Migrated from github.com) commented

2018-03-21 14:34:49 +00:00

Why not wrap this function inside the promise, so you don't have to hold references to resolve and reject.


return new Promise((resolve, reject) => {
  this._queue.push({ method, data, resolve, reject });
  process.nextTick(this._runQueue.bind(this));
});

Why not wrap this function inside the promise, so you don't have to hold references to `resolve` and `reject`. ```javascript return new Promise((resolve, reject) => { this._queue.push({ method, data, resolve, reject }); process.nextTick(this._runQueue.bind(this)); }); ```

laurynas-karvelis (Migrated from github.com) reviewed 2018-03-21 14:42:27 +00:00

src/functions/api.js

laurynas-karvelis (Migrated from github.com) commented

2018-03-21 14:42:27 +00:00

You are right, after re-running same test file that sends out multiple messages got same result without any issues. Appears there's no negative impact reducing this code excerpt you suggested 👍
Added your suggestions in a new commit

You are right, after re-running same test file that sends out multiple messages got same result without any issues. Appears there's no negative impact reducing this code excerpt you suggested :+1: Added your suggestions in a new commit

mdibaiee commented

2018-03-21 14:49:23 +00:00

(Migrated from github.com)

Looks good to me, just to make sure, have you tested this thoroughly? Asking since this can have implications on most parts of the application. If so, good to go by me, thanks a lot! :D

laurynas-karvelis commented

2018-03-21 14:51:32 +00:00

(Migrated from github.com)

I have tested it out with simple Messages, no document uploads etc. But from the tests I've done, it worked like a charm for me.

mdibaiee commented

2018-03-21 14:53:20 +00:00

(Migrated from github.com)

Could you please put some time to test other types of messages just to make sure? I would be grateful 🙏

Could you please put some time to test other types of messages just to make sure? I would be grateful :pray:

laurynas-karvelis commented

2018-03-21 14:58:07 +00:00

(Migrated from github.com)

As every API method is just a wrapper for API.request method, and param count to it remains the same, I see no reason other child classes of Base should behave differently. I haven't dealt with sending docs etc via Telegram API before. Could be a possibility for you to test this out quicker than me by any chance? :)

mdibaiee commented

2018-03-21 14:59:48 +00:00

(Migrated from github.com)

I guess you are right, since the change only affects the way the method is called rather than processed, it shouldn't make any difference among different types of messages.

Thanks a lot Laurynas! 👍

I guess you are right, since the change only affects the way the method is _called_ rather than processed, it shouldn't make any difference among different types of messages. Thanks a lot Laurynas! :+1:

laurynas-karvelis commented

2018-03-21 15:01:51 +00:00

(Migrated from github.com)

🎉

:tada:

mdibaiee commented

2018-03-21 15:02:04 +00:00

(Migrated from github.com)

Published telegram-api@4.0.0 🎉👍

Published `telegram-api@4.0.0` :tada::+1:

Sign in to join this conversation.

No reviewers