ArkEcosystem / core

The ARK Core Blockchain Framework. Check https://learn.ark.dev for more information.
https://ark.io
MIT License
338 stars 285 forks source link

EventEmitter interface inconsistent #2392

Closed Nigui closed 5 years ago

Nigui commented 5 years ago

Describe the bug

EventEmitter interface is based on NodeJS events module.

core-event-emitter module returns custom EventEmitter class which does not implement interface and native emitter is not accessible (private).

So, it's possible to have the following error when calling native emitter functions not implemented by custom emitter:

(node:89665) UnhandledPromiseRejectionWarning: TypeError: emitter.off is not a function

To Reproduce In my example I tried to call the off function of native emitter.

Expected behavior Either a typescript error must be thrown at compile time. Or off function of native emitter is available on custom class Or all native emitter function are available.

I can make the fix asap after best solution has been chosen:

ghost commented 5 years ago

Thanks for opening this issue! A maintainer will review this in the next few days and explicitly select labels so you know what's going on.

If no reviewer appears after a week, a reminder will be sent out.

faustbrian commented 5 years ago

https://github.com/ArkEcosystem/core/pull/2396

ghost commented 5 years ago

This issue has been closed. If you wish to re-open it please provide additional information.

dlecan commented 5 years ago

@faustbrian thank you for fixing this quickly.

It would have been also great to discuss it a little bit more before.

1/ The done "bugfix" doesn't really fix the underlying issue: the Ark emitter.EventEmitter type is not a NodeJS events.EventEmitter and several other methods are missing, such as removeAllListeners, listeners, addListener ... These methods used to be available with eventemitter3, removed by #2329, which have introduced an API breaking change and have impacted our Unik-Name implementation of Ark core (see https://github.com/ArkEcosystem/AIPs/issues/70).

Another solution: emitter.EventEmitter could inherit from events.EventEmitter and override a few methods to update the max count of listeners.

2/ @nigui had already done a private fix when he described the issue. So a fix for this issue has been done twice. This can bee seen as "lost time". Was this issue critical enough to fix it so quickly (5h)?

So, for the next time, can you please tell us:

Thank you

faustbrian commented 5 years ago

If you encounter an issue and already have a solution just submit a PR with it otherwise small issues like that will just be resolved.