Introduce internal API call queuing mechanism #43

Merged
laurynas-karvelis merged 2 commits from master into master 2018-03-21 14:59:55 +00:00
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)

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:
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

Thanks! This is definitely a welcome improvement, there is a single detail I've commented below
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
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

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.

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? :)

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
No Milestone
No project
No Assignees
1 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: thereadme/node-telegram-api#43
No description provided.