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

feat(websocket): add ability to remove listener #2027

Open
wants to merge 9 commits into
base: v2
Choose a base branch
from

Conversation

twlite
Copy link
Contributor

@twlite twlite commented Nov 10, 2024

This pr changes listeners array to use a set instead, which prevents addition of duplicated listeners which could possibly lead to memory leaks.

Change listeners array to use a set instead, which prevents addition of duplicated listeners which could possibly lead to memory leaks.
@twlite twlite requested a review from a team as a code owner November 10, 2024 05:17
Copy link
Contributor

github-actions bot commented Nov 11, 2024

Package Changes Through 5d6a22c

There are 2 changes which include websocket-js with minor, websocket with minor

Planned Package Versions

The following package releases are the planned based on the context of changes in this pull request.

package current next
websocket 2.2.1 2.3.0
websocket-js 2.2.1 2.3.0

Add another change file through the GitHub UI by following this link.


Read about change files or the docs at github.com/jbolda/covector

@FabianLars
Copy link
Member

i agree that a way to remove listeners is needed (that's why i didn't close the issue) but i'm not a fan of the Set here (or how it was used). I can very much imagine that someone wants to register the same function twice for whatever reason. Buuuut i'm totally fine with doing it this way and waiting to see if anyone actually complains.

Also, changing the return value is technically a breaking change but because it didn't return anything before and because this plugin doesn't have that many users i think we can get away with it.

FabianLars
FabianLars previously approved these changes Nov 14, 2024
@twlite
Copy link
Contributor Author

twlite commented Nov 14, 2024

Also, changing the return value is technically a breaking change but because it didn't return anything before and because this plugin doesn't have that many users i think we can get away with it.

Yes, that's exactly what I thought 😅

i'm not a fan of the Set here (or how it was used). I can very much imagine that someone wants to register the same function twice for whatever reason.

Yeah but it doesn't make sense to do so because same function gets invoked multiple times which can cause weird behaviors.

@jffaust
Copy link

jffaust commented Jan 13, 2025

The ability to remove a listener is pretty important in my opinion. Otherwise, you'd have to close the socket but you might still need the connection for other purposes. I say let's proceed with these changes.

@twlite do you think you can look at the merge conflicts?

@twlite twlite requested a review from FabianLars January 13, 2025 18:38
@twlite
Copy link
Contributor Author

twlite commented Jan 13, 2025

@twlite do you think you can look at the merge conflicts?

done

@twlite
Copy link
Contributor Author

twlite commented Jan 13, 2025

Looks like we got an error saying Missing "minor" bump for Rust package "websocket" in .changes/cleanup-fn-websocket.md.. I have not touched the rust part here, do I still have to add it?

@FabianLars
Copy link
Member

yes. since 2.2.0 the rust and js package must have matching versions (added the workflow at the same time to train ourselves to remember that)

@jffaust
Copy link

jffaust commented Jan 17, 2025

Are we good to go @FabianLars ? I looked around the documentation but I couldn't find any details about the release schedule/mechanism. Will merging this PR automatically create a new version of the package on NPM?

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

Successfully merging this pull request may close these issues.

3 participants