Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Replace native events with eventemitter3 #3330

Closed
wants to merge 3 commits into from
Closed

Conversation

acolytec3
Copy link
Contributor

This PR is an exploration of replacing eventEmitter with eventemitter3 to improve browser compatibility since the native nodejs Event Emitter seems to have some typing issues/interface variance from the browser equivalent.

  • Known impacts - requires removing get/setMaxListeners and prependListener/Once

  • There is some type difference in the generics where our current use of <E extends keyod T> in asyncEventEmitter that causes Typescript to complain

  • The once function on the async event emitter does not appear to work properly now. It appears that the step in the process where it removes the listener after executiong the calback never occurs.

@acolytec3 acolytec3 added dependencies Pull requests that update a dependency file package: common package: client package: util labels Mar 19, 2024
@acolytec3 acolytec3 marked this pull request as draft March 19, 2024 16:45
@acolytec3 acolytec3 mentioned this pull request Mar 19, 2024
@holgerd77
Copy link
Member

Starting to think that we might want to move this eventemitter thing to the next breaking release round. This is absolutely not urgent and we would cause less hazzle here.

I am loosely targeting a breaking release round in autumn, September or so, then we can collect some more stuff and start a condensed update period after the Summer holidays.

@acolytec3
Copy link
Contributor Author

That would be fine. I'm still not 100% convinced this is the right approach anyway. There are some odd typing issues I'm trying to sort out at the moment to at if I can make it all fit together.

@acolytec3
Copy link
Contributor Author

After merging the vitest 1.0 PR, doesnt't seem like it makes sense to pursue this further since eventemitter3's API isn't a dropin replacement for native events as claimed (see the changes I had to make to asyncEventEmitter

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants