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

Making Websocket Local read-write #676

Merged
merged 2 commits into from
Sep 13, 2023

Conversation

supagroova
Copy link
Contributor

@supagroova supagroova commented Jul 6, 2023

Fixes #675

@ReneWerner87
Copy link
Member

ReneWerner87 commented Jul 6, 2023

Thanks for that, can you add the signatures to the readme like the other middlewares?
we plan to improve the readme's and also make them available in our online fiber doc

https://github.com/gofiber/contrib/tree/main/swagger#signatures
....

https://github.com/gofiber/fiber/blob/453ccadadd09b80155cdde7cc9d1beae9ec91f21/docs/partials/routing/handler.md?plain=1#L8-L26

@gaby
Copy link
Member

gaby commented Jul 6, 2023

This should wait for #669 so we can test it

@ReneWerner87
Copy link
Member

@supagroova can you pls update your branch with the upstream master

@ReneWerner87
Copy link
Member

@supagroova friendly ping

@gaby
Copy link
Member

gaby commented Aug 10, 2023

@ReneWerner87 I took care of the merge conflict, do we need another unit-test for these changes? and the README changes.

@ReneWerner87
Copy link
Member

@ReneWerner87 I took care of the merge conflict, do we need another unit-test for these changes? and the README changes.

yes would be good

@gaby gaby self-assigned this Aug 15, 2023
@gaby
Copy link
Member

gaby commented Aug 19, 2023

I honestly have no idea how to test this, maybe @mstrYoda can help with this unit-test

@mstrYoda
Copy link
Member

I honestly have no idea how to test this, maybe @mstrYoda can help with this unit-test

Well I think adding the new if condition as a test scenario to here works well: https://github.com/gofiber/contrib/blob/main/websocket/websocket_test.go#L224

@ReneWerner87
Copy link
Member

@mstrYoda @gaby is it ready for merge ?

@mstrYoda
Copy link
Member

mstrYoda commented Sep 5, 2023

@mstrYoda @gaby is it ready for merge ?

May be we can add unit tests here 🤔 Or we can just merge it as is.

@gaby
Copy link
Member

gaby commented Sep 5, 2023

I was going to add tests, then i realize i have no idea how to test this 😂

@mstrYoda
Copy link
Member

I was going to add tests, then i realize i have no idea how to test this 😂

Well I will add test for this function no worry, we can merge this PR 👍

@ReneWerner87 ReneWerner87 merged commit 502f4c5 into gofiber:main Sep 13, 2023
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

🤗 [Question]: Must Websocket Locals be read-only?
4 participants