In my particular use case I might be sending many messages to a contact from bot without ever expecting an answer to sent message. I've noticed that only Question class is actually dealing with question:answer event. Otherwise a process can end up having many event listeners stacking up in the EventEmitter's instance without being actually called as there's no response from the contact ever, thus triggering nodejs throwing an error like (this is after me increasing event listener max count):
MaxListenersExceededWarning: Possible EventEmitter memory leak detected. 201 update listeners added. Use emitter.setMaxListeners() to increase limit
In my particular use case I might be sending many messages to a contact from bot without ever expecting an answer to sent message. I've noticed that only Question class is actually dealing with `question:answer` event. Otherwise a process can end up having many event listeners stacking up in the EventEmitter's instance without being actually called as there's no response from the contact ever, thus triggering nodejs throwing an error like (this is *after* me increasing event listener max count):
```
MaxListenersExceededWarning: Possible EventEmitter memory leak detected. 201 update listeners added. Use emitter.setMaxListeners() to increase limit
```
mdibaiee
(Migrated from github.com)
requested changes 2018-12-24 11:37:04 +00:00
mdibaiee
(Migrated from github.com)
left a comment
Copy Link
Copy Source
Hey!
Your argument and use case are valid, but I'd suggest we avoid breaking changes in this repository. To be honest, since the module is not actively maintained anymore, breaking changes can be much more problematic for users and for maintainers. See my comments below.
But generally, thanks for the pull-request!
Hey!
Your argument and use case are valid, but I'd suggest we avoid breaking changes in this repository. To be honest, since the module is not actively maintained anymore, breaking changes can be much more problematic for users and for maintainers. See my comments below.
But generally, thanks for the pull-request!
This is a breaking change, I would default to true, and in your use case, you could avoid unnecessary listeners by setting the value to false explicitly.
It might actually be the right thing to do to only listen when explicitly asked, but since this is a breaking change, I'd say we choose the safe path for now. If necessary, a major release can break this.
This is a breaking change, I would default to `true`, and in your use case, you could avoid unnecessary listeners by setting the value to false explicitly.
It might actually be the right thing to do to only listen when explicitly asked, but since this is a breaking change, I'd say we choose the safe path for now. If necessary, a major release can break this.
Eslinter was complaining about inconsistent return point, hence needed to return something, otherwise as per your resolve(); return; I'll add a new commit per both your comments. Thanks!
Eslinter was complaining about inconsistent return point, hence needed to return something, otherwise as per your `resolve(); return;` I'll add a new commit per both your comments. Thanks!
laurynas-karvelis
(Migrated from github.com)
reviewed 2018-12-24 11:59:16 +00:00
laurynas-karvelis
(Migrated from github.com)
left a comment
Copy Link
Copy Source
All requested changes done :)
All requested changes done :)
mdibaiee
(Migrated from github.com)
approved these changes 2018-12-24 11:59:35 +00:00
mdibaiee
(Migrated from github.com)
left a comment
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
In my particular use case I might be sending many messages to a contact from bot without ever expecting an answer to sent message. I've noticed that only Question class is actually dealing with
question:answerevent. Otherwise a process can end up having many event listeners stacking up in the EventEmitter's instance without being actually called as there's no response from the contact ever, thus triggering nodejs throwing an error like (this is after me increasing event listener max count):Hey!
Your argument and use case are valid, but I'd suggest we avoid breaking changes in this repository. To be honest, since the module is not actively maintained anymore, breaking changes can be much more problematic for users and for maintainers. See my comments below.
But generally, thanks for the pull-request!
This is a breaking change, I would default to
true, and in your use case, you could avoid unnecessary listeners by setting the value to false explicitly.It might actually be the right thing to do to only listen when explicitly asked, but since this is a breaking change, I'd say we choose the safe path for now. If necessary, a major release can break this.
Is this to escape the linter? Instead I think you should avoid returning
resolve(), but rather:Eslinter was complaining about inconsistent return point, hence needed to return something, otherwise as per your
resolve(); return;I'll add a new commit per both your comments. Thanks!All requested changes done :)
Looks good to me, thank you!
Published as
telegram-api@4.2.0.