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

Transfer socketio middleware to contrib #956

Merged
merged 20 commits into from
Feb 18, 2024
Merged

Conversation

antoniodipinto
Copy link
Contributor

Hey @ReneWerner87 can you please review this PR so ikisocket will move under contrib repo?

After the pull request will be accepted I will move also the examples to recipes

Thank you!

@ReneWerner87
Copy link
Member

Sure

@ReneWerner87 ReneWerner87 added the ✏️ Feature New feature or request label Feb 9, 2024
@ReneWerner87
Copy link
Member

ReneWerner87 commented Feb 9, 2024

@antoniodipinto
pls add some workflows for this new pkg
and also a link in the root README.md

  • add release drafter config and flow
  • add pkg in dependabot
  • add test workflow
  • add link in the root Readme.md
  • use the same indentations in the readme of the pkg as in the other packages, currently it looks a bit different

@antoniodipinto
Copy link
Contributor Author

@ReneWerner87 should be ok now, please check whenever you're able.

Thanks!

antoniodipinto and others added 4 commits February 9, 2024 14:05
@ReneWerner87
Copy link
Member

@gaby @efectn any comments , open tasks?

@antoniodipinto
Copy link
Contributor Author

Thanks for your support @ReneWerner87

ikisocket/ikisocket.go Outdated Show resolved Hide resolved
@gaby
Copy link
Member

gaby commented Feb 9, 2024

Could we consider a different name for this middleware?

  • contrib/socket
  • contrib/events

.github/workflows/test-ikisocket.yml Outdated Show resolved Hide resolved
ikisocket/go.mod Outdated Show resolved Hide resolved
ikisocket/go.mod Outdated Show resolved Hide resolved
ikisocket/go.mod Outdated Show resolved Hide resolved
@gaby gaby changed the title transfer ikisocket to contrib Transfer ikisocket to contrib Feb 9, 2024
@gaby gaby changed the title Transfer ikisocket to contrib Transfer ikisocket middleware to contrib Feb 9, 2024
@antoniodipinto
Copy link
Contributor Author

Hey guys, the only error that I can see it's related to the linter

Error: ikisocket.go:202:20: func `(*safePool).reset` is unused (unused)
func (p *safePool) reset() {
                   ^
Error: ikisocket.go:43[6](https://github.com/gofiber/contrib/actions/runs/7844957650/job/21408365949?pr=956#step:5:7):23: func `(*Websocket).queueLength` is unused (unused)
func (kws *Websocket) queueLength() int {

These functions are necessary for the tests not for the normal usage of the library

@ReneWerner87
Copy link
Member

ReneWerner87 commented Feb 12, 2024

et) queueLength() int {

then disable the error with nolint and leave a comment type there for use
https://golangci-lint.run/usage/false-positives/#nolint-directive

Could we consider a different name for this middleware?

  • contrib/socket
  • contrib/events

@antoniodipinto what do you think?

@antoniodipinto
Copy link
Contributor Author

Hi @ReneWerner87 you can change the same as you want, I'm not attached to the ikisocket name. The only concern that I have is that socket and events can be misunderstood.
Maybe a different name that can prevent misunderstandings, something like ws_fiber or socket.io, not these names ofc.
My library it's still a wrapper on top of Fiber's websocket, what do you think?

@ReneWerner87
Copy link
Member

@gofiber/maintainers any good ideas for a name?

@gaby
Copy link
Member

gaby commented Feb 13, 2024

@gofiber/maintainers any good ideas for a name?

Socket.IO is also a protocol. So we could use: gofiber/contrib/socket.io

This middleware covers almost the full protocol.

https://github.com/socketio/socket.io-protocol

ikisocket/ikisocket.go Outdated Show resolved Hide resolved
ikisocket/ikisocket.go Outdated Show resolved Hide resolved
@gaby
Copy link
Member

gaby commented Feb 13, 2024

These are the suggested names by ChatGPT after reviewing the code:

  1. advancedwebsocket: This name indicates that the middleware provides advanced WebSocket capabilities beyond the basic functionalities.

  2. websocketplus: The "plus" suffix suggests additional features or improvements over the standard WebSocket middleware.

  3. websocketx: Using "X" often denotes an extended or expanded version of something, implying this middleware offers more than the basic implementation.

  4. eventsocket: If the middleware's standout feature is its event-driven approach to WebSocket communication, this name highlights that aspect.

  5. socket.io: If the middleware aims to provide functionalities similar to Socket.IO (like fallbacks, rooms, namespaces, etc.), naming it after Socket.IO with a Fiber-specific prefix could make sense.

  6. realtime: This name focuses on the purpose of the middleware rather than the technology, emphasizing its use for real-time communication.

  7. socketlayer: This suggests the middleware adds an additional layer of functionality or abstraction on top of basic WebSocket communication.

  8. live: Short and to the point, this name emphasizes the middleware's role in enabling live, real-time interactions.

@antoniodipinto
Copy link
Contributor Author

live and socket.io seems to be good and pretty self-explanatory

@efectn
Copy link
Member

efectn commented Feb 13, 2024

socket.io seems good to me

@ReneWerner87
Copy link
Member

  • ok then lets rename everything to socket.io

@gaby
Copy link
Member

gaby commented Feb 15, 2024

@antoniodipinto We you get a chance, can you update the code with the new name. Should be good to merge after that :-)

@antoniodipinto
Copy link
Contributor Author

@gaby name changed

@ReneWerner87 ReneWerner87 changed the title Transfer ikisocket middleware to contrib Transfer socketio middleware to contrib Feb 18, 2024
@ReneWerner87 ReneWerner87 merged commit b6746cc into gofiber:main Feb 18, 2024
55 checks passed
@ReneWerner87
Copy link
Member

@antoniodipinto thx
https://github.com/gofiber/contrib/releases/tag/socketio%2Fv1.0.0

now you can start with

After the pull request will be accepted I will move also the examples to recipes

@ReneWerner87
Copy link
Member

@antoniodipinto Have you already moved the examples?

@antoniodipinto
Copy link
Contributor Author

I'll move it later this week

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
✏️ Feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants