feature: add event listeners to get answers from Base.send method only when explicitly asked #47

2018-12-24T09:48:53Z

laurynas-karvelis commented

2018-12-24 09:48:53 +00:00

(Migrated from github.com)

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

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!

src/types/Base.js

mdibaiee (Migrated from github.com) commented

2018-12-24 11:33:47 +00:00

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.

src/types/Base.js

mdibaiee (Migrated from github.com) commented

2018-12-24 11:35:15 +00:00

Is this to escape the linter? Instead I think you should avoid returning resolve(), but rather:

resolve();
return

Is this to escape the linter? Instead I think you should avoid returning `resolve()`, but rather: ``` resolve(); return ```

laurynas-karvelis (Migrated from github.com) reviewed 2018-12-24 11:54:12 +00:00

src/types/Base.js

laurynas-karvelis (Migrated from github.com) commented

2018-12-24 11:54:12 +00:00

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!