Morglod / tseep

Fastest event emitter in the world for js (and only 381 bytes in build)
https://github.com/Morglod/tseep/blob/master/benchmarks/README.md
MIT License
174 stars 5 forks source link

`prependOnceListener` does not work as expected #21

Closed mustafa519 closed 4 months ago

mustafa519 commented 7 months ago

Hello,

I am having an issue about once listeners. Seems like prependOnceListener overrides all the once events. In the below example, it never logs "firstOnce" if I set prependOnceListener within once.

const ee = new EventEmitter();

ee.once('onceTest', () => { console.log('firstOnce') });

// ee.once('onceTest', () => { console.log('secondOnce') });
// ee.prependOnceListener('onceTest', () => { console.log('prependOnce') });

ee.prependOnceListener('onceTest', () => { console.log('prependOnce again') });

ee.emit('onceTest', 'yoo!');

The other problem is I cannot set multiple once events if I set prependOnceListener. If I uncomment them it's going to cause an error(FIXME) that you had pointed https://github.com/Morglod/tseep/blob/master/src/ee.ts#L216 here.

Mustafa, Thanks.

Morglod commented 7 months ago

@mustafa519 Hi! Thank you for finding this bug, fixed it in new version 1.2.1 https://github.com/Morglod/tseep/commit/37575bdaceb61e42d3b6704bc93e4f7b22637454

mustafa519 commented 7 months ago

Awesome! That was fast. I can confirm that the fix is working like a charm. I have a few things that I'd ask you to consider them.

Changelog/Releases

I see I am behind two versions that I had no idea, even if I am watching the repository. Could you add create releases when you publish a new version, so we may want to update our packages immediately to get the benefits of the new versions.

A helper package like np can help to manage them quickly. Surely, that's going to create a history of the repo to get more interactions with your amazing work.

Extending the DefaultEventMap type

The __proto__ type guard doesn't allow me to extend DefaultEventMap which causes an error.

image

if I don't extend then it causes like I have to use the same pattern as DefaultEventMap requires itself

image

Since not all people use Typescript, would you consider adding a simple if check guard on the development mode?

// I wrapped with another condition that people may use bundler, which is going to be removed as unused code on the production builds.
if (process.env.NODE_ENV === 'development')
{
  if (eventName === '__proto__') throw new Error('"__proto__" is a banned event name.');
}

Thus, it won't cause overhead on the production mode, which is going to be fast as it is.

There are not a lot of people who could develop the low level stuffs deeply like you. So, I wanted to support you as much as I can.

Thank you for the support!

Morglod commented 5 months ago

On latest typescript version everyting works ok (^5.4.5):

interface MyEvents extends DefaultEventMap {
    x: () => void,
    y: () => void,
}

const ee = new EventEmitter<MyEvents>();

What typescript version you have? @mustafa519 And what is in tsconfig


I believe that if you use plain javascript, than you should read documentation. Also having __proto__ event name is reeaaaallyyyyyy rare case. And if developer uses __proto__ somewhere, he should keep in mind that it may cause problems. Also its anywhay will be handled with IDE type analyzer. Also __proto__ event name actually works, but you will get bad performance.

var events = {};
events.aa = () => console.log('aa');
events.__proto__ = () => console.log('bb');
events['aa']();
events['__proto__']();

// output:
// aa
// bb