Melkeydev / go-blueprint

Go-blueprint allows users to spin up a quick Go project using a popular framework
https://docs.go-blueprint.dev/
MIT License
5.98k stars 337 forks source link

A concept implementation of a websocket endpoint #27

Closed MitchellBerend closed 1 year ago

MitchellBerend commented 1 year ago

Resolves #25

This pr adds a websocket endpoint for all frameworks.

MitchellBerend commented 1 year ago

The current implementation does not have any error handling at all, which is obviously not good. I think either a 5xx should be returned or it should be very clear that errors are not handled at all.

Should I just return a 5xx when some error is encountered?

MitchellBerend commented 1 year ago

The current implementation does not have any error handling at all, which is obviously not good. I think returning a 5xx is the best option but it might lead to users not looking at the implementation since it "works" as is.

As this is still a wip I have 2 questions as for the direction of this pr

Should this handler just return a 5xx when any error is encountered? Should there be a comment about specifically looking at the error handling in this endpoint?

I can see a point for doing both but I'd like your opinion on that too before I keep working on this.

MitchellBerend commented 1 year ago

Gorilla/mux has their own websocket implementation which could be used for the generated code. Something that has to be decided is if gorilla/mux is also going to use a different websocket implementation than the rest. Apart from fiber which also uses it's own implementation, all the other implementations use the same websocket library. I think an argument could be made for keeping the generated code in the same ecosystem as much as possible.

Let me know what you think.

MitchellBerend commented 1 year ago

I slept on the error logging decision and I decided that it should not be up to the user to implement the error handling for the generated code. I had some trouble getting the fiber websocket endpoint implementation working yesterday and adding error logging there helped a lot.

MitchellBerend commented 1 year ago

Okay I decided to push back the possible gorilla/websocket implementation since this pr has gotten quite big already and I don't want to keep expanding it's scope.

Melkeydev commented 1 year ago

This looks good. I am going to test it out right now and check out the code. I will give my opinion on the error handling now too. But there is also a merge conflict that needs to be resolved

MitchellBerend commented 1 year ago

I think all of the points are addressed but I still need to test and format the code properly since I can't do that on this computer.

itschip commented 1 year ago

We might also want to send a close message and gracefully shutdown the websocket.

EDIT: Unless that is something we just let the end-user do themselves?

MitchellBerend commented 1 year ago

We might also want to send a close message and gracefully shutdown the websocket.

This sounds like a good idea tbh. If this PR gets accepted I think a new PR for that would be appropriate since this PR is large enough as it is imo.

MitchellBerend commented 1 year ago

Oh sorry I forgot to test this at home...