Switch events to eventemitter3 - the saga #3746
Conversation
acolytec3
changed the title
Codecov ReportAttention: Patch coverage is
Additional details and impacted files
Flags with carried forward coverage won't be shown. Click here to find out more. |
acolytec3
added
package: blockchain
package: vm
package: common
package: client
package: util
package: evm
dependencies
There was a problem hiding this comment.
Looks clean and good, however, I see that for VM and EVM there are now changes where it seems that even on sync events resolve() is added and called (in the tests). These should be removed.
Also, the "breaking change" here is that blockchain events do now not support async events. Not sure if we want to put that back in, but if anyone is listening async on deletedCanonicalBlocks we definitely should.
I will also test this ASAP :)
packages/client/src/sync/fullsync.ts
Outdated
| this.config.events.once(Event.SYNC_SYNCHRONIZED, (height?: number) => { | ||
| this.resolveSync(height) | ||
| this.config.events.once(Event.SYNC_SYNCHRONIZED, (chainHeight?: bigint) => { | ||
| this.resolveSync(Number(chainHeight)) |
There was a problem hiding this comment.
We could also propagate the bigint here?
There was a problem hiding this comment.
Will make this change. Not really sure why we even propagate this other than for debug logging.
| [Event.POOL_PEER_BANNED]: [bannedPeer: Peer] | ||
| [Event.PROTOCOL_ERROR]: [boundProtocolError: Error, peerCausingError: Peer] | ||
| [Event.PROTOCOL_MESSAGE]: [messageDetails: any, protocolName: string, sendingPeer: Peer] | ||
| } |
There was a problem hiding this comment.
How does the deletion of this event bus change the typing of the client types?
There was a problem hiding this comment.
eventEmitter3 doesn't need this additional layer of type expansions and we can just use the basic EventParams interface keyed by the properties of the Event enum.
| assert.throws(() => sender.sendStatus({ id: 5 }), /not a function/, 'sendStatus error') | ||
| }) | ||
| it('throws sendMessage error', () => { | ||
| assert.throws(() => sender.sendMessage(1, 5), /not a function/, 'sendMessage error') |
There was a problem hiding this comment.
These events are deleted and do not seem related (?) to events, is this intended?
There was a problem hiding this comment.
Right, they aren't directly related but they're pointless tests and don't actually test anything other than that when you try to access a non-existent property on an object that Javascript throws. I just stumbled across it because it referenced EventEmitter.
| describe('should receive message', async () => { | ||
| const rlpxProtocol = { events: new EventEmitter() } | ||
| const sender = new RlpxSender(rlpxProtocol as Devp2pETH) | ||
| const sender = new RlpxSender(rlpxProtocol as never as Devp2pETH) |
There was a problem hiding this comment.
Why do we have to cast it as never first now?
There was a problem hiding this comment.
I've reversed this edit. For some reason, Typescript wasn't paying nicely with it before.
| listener(data, resolve) | ||
| }) | ||
| } else { | ||
| listener(data) |
There was a problem hiding this comment.
This is the async logic handler now, right? This is not part of eventemitter3?
There was a problem hiding this comment.
Correct, if the listener has 2 arguments, we assume it's looking for the event object and then the resolve function (and therefore is async). Otherwise, we just call the listener.
| // Setup an event listener on the `afterTx` event | ||
| vm.events.on('afterTx', (event, resolve) => { | ||
| console.log('asynchronous listener to afterTx', bytesToHex(event.transaction.hash())) | ||
| resolve?.() |
There was a problem hiding this comment.
An explicit note that resolve must be called would be nice here :)
| resolveSync(height?: bigint) { | ||
| this.clearFetcher() | ||
| const heightStr = typeof height === 'number' && height !== 0 ? ` height=${height}` : '' | ||
| const heightStr = typeof height === 'bigint' && height !== BIGINT_0 ? ` height=${height}` : '' |
There was a problem hiding this comment.
Do these things even solve any bugs?? Or did you just also change the type coming in?
|
Really really really nice. |
4 participants
After failed attempt after attempt, I'm determined to get this right.
eventemitter3is the right choice for us on a performance basis since it's 50% faster than native Nodejs events.Now, I just need to take on the final boss of async events witheventemitter3.Final boss has been defeated with the help of the robots.