ethereumjs / ethereumjs-monorepo

Monorepo for the Ethereum VM TypeScript Implementation
2.58k stars 750 forks source link

Monorepo: Replace Node.js EventEmitter #3636

Open holgerd77 opened 1 week ago

holgerd77 commented 1 week ago

The Node.js native EventEmitter is the last Node.js primitive we still use in our libraries. This causes issues like #3439 and we should replace with a Node.js independent solution.

To summarize the current situation.

The basic EventEmitter from Node is used in:

Then we have an AsyncEventEmitter integrated in Util which builds upon the Node.js event emitter and adds (correct me if I am wrong?) the functionality to "stop within events" (actually I am so so unsure what this thing actually adds, please someone clarify 😂 ).

This AsyncEventEmitter is then used in:

These event emitter usages should be removed by a - dependency-free - event emitter solution. I would find it most charming if we could eliminate this EventEmitter/AsyncEventEmitter separation and use one solution for both.

I found the following packages:

I did not found any meaningful TypeScript event emitter solutions, also, this list might not be complete.

EventEmitter2 might be a good choice having an explicit async mode (per flag), so it might suit an integrated approach well. But since I am understanding the requirements so badly I cannot help much atm with the choice otherwise TBH. 😬

@acolytec3 is already relatively deep in the topic and we already had several exchanges on that, so I guess it would be best if he takes this on.

holgerd77 commented 1 week ago

//cc @wighawag

holgerd77 commented 1 week ago

Some Remix usage of events here.

acolytec3 commented 1 week ago

A few comments for posterity/discussion: 1) The main use of the AsyncEventEmitter class is for pausing bytecode execution to be able to evaluate the state of the EVM at that specific point in execution (similarly to how our stepHook function runs where you can access the stack/memory/metadata for each opcode as its processed). 2) I already did an initial exploration of eventemitter3 and it wouldn't work for us unless they have an async mode I'm not aware of. I took a first stab at trying to integrate it and use inside of our AsyncEventEmitter class and there were some edge cases in the API that were not 1-1 with the native EventEmitter class. See #3330

holgerd77 commented 1 week ago

On 2.: Ok, but my idea would be that we would throw the AsyncEventEmitter class away?

lukastanisic99 commented 1 week ago

Couldn't the eventemitter3 be wrapped/extended with additional functionality to mimic the nodejs native EventEmitter?