Add subscription #47
Add subscription #47
Conversation
Codecov Report
@@ Coverage Diff @@
## main #47 +/- ##
==========================================
- Coverage 89.22% 88.48% -0.75%
==========================================
Files 32 32
Lines 399 408 +9
==========================================
+ Hits 356 361 +5
- Misses 43 47 +4
Continue to review full report in Codecov by Sentry.
|
roznawsk
force-pushed
the
RTC-299-add-subscription
branch
from
3ddd57d
to
df3712c
Compare
.gitmodules
Outdated
| @@ -1,5 +1,5 @@ | |||
| [submodule "protos"] | |||
| path = protos | |||
| url = https://github.com/jellyfish-dev/protos.git | |||
| branch = master | |||
| branch = actually-subscribe-for-notifications | |||
lib/jellyfish_web/server_socket.ex
Outdated
| room_state = get_room_state(option) | ||
|
|
||
| %ServerMessage{content: room_state} | ||
| |> ServerMessage.encode() | ||
| |> then(&{:ok, &1}) |
There was a problem hiding this comment.
I think that this is more readable (although its like 60% for this, 40% for what you wrote, it's up to you to decide):
| room_state = get_room_state(option) | |
| %ServerMessage{content: room_state} | |
| |> ServerMessage.encode() | |
| |> then(&{:ok, &1}) | |
| room_state = get_room_state(option) | |
| msg = | |
| %ServerMessage{content: room_state} | |
| |> ServerMessage.encode() | |
| {:ok, msg} |
That's a nitpick tho.
lib/jellyfish_web/server_socket.ex
Outdated
| defp handle_request(request), do: request | ||
|
|
There was a problem hiding this comment.
I would remove the default case all together, or at least rise, we don't expect any other messages.
There was a problem hiding this comment.
We do raise, in the handle_in callback.
In this clause we return request instead of {:ok, mesage} so we raise in the with.
However this isn't explicit really.
There was a problem hiding this comment.
I agree with @LVala on this -- I'd say either remove the default case, explicitly raise, or return something like {:error, :unknown_request} (and maybe log an error); either of these solutions is fine by me
| fn -> | ||
| conn = delete(conn, ~p"/room/#{room_id}/") | ||
| assert response(conn, :no_content) | ||
| end |
There was a problem hiding this comment.
This seems hacky, maybe let's create on_exit callback in setup block for the whole file which simply deletes all of the rooms. I considered doing this before but it wasn't necessary back then.
|
I think the logic in |
| @@ -83,13 +84,7 @@ defmodule JellyfishWeb.ServerSocket do | |||
|
|
|||
| def handle_in({encoded_message, [opcode: :binary]}, state) do | |||
There was a problem hiding this comment.
| def handle_in({encoded_message, [opcode: :binary]}, state) do | |
| @impl true | |
| def handle_in({encoded_message, [opcode: :binary]}, state) do |
There was a problem hiding this comment.
Nitpick; might as well annotate the handle_in callback from line 102.
There was a problem hiding this comment.
tbh, I prefer to make the annotations only once - above the first callback clause, but if everyone prefers adding annotations before each callback I will change this
There was a problem hiding this comment.
I think it'd be clearer this way, GPT4 seems to agree, but ultimately it's your decision
lib/jellyfish_web/server_socket.ex
Outdated
| defp handle_request(request), do: request | ||
|
|
There was a problem hiding this comment.
I agree with @LVala on this -- I'd say either remove the default case, explicitly raise, or return something like {:error, :unknown_request} (and maybe log an error); either of these solutions is fine by me
lib/jellyfish_web/server_socket.ex
Outdated
| {:ok, msg} | ||
| end | ||
|
|
||
| defp handle_subscribe(request), do: request |
| rooms | ||
| |> Enum.each(fn %{"id" => id} -> | ||
| conn = delete(conn, ~p"/room/#{id}") | ||
| response(conn, 204) |
Receiving notifications requires subscribing to the topic first.
Ref: