Subscribing for events #19
Conversation
lib/jellyfish/notifier.ex
Outdated
| iex> {:ok, notifier} = Jellyfish.Notifier.start(events: [:server_notifications]) | ||
| {:ok, #PID<0.301.0>} | ||
| iex> {:ok, _rooms} = Jellyfish.Notifier.subscribe(notifier, :all) | ||
|
|
||
| # Subscribe current process to all server notifications. | ||
| iex> {:ok, _rooms} = Jellyfish.Notifier.subscribe(notifier, :server_notifications, :all) |
There was a problem hiding this comment.
The fact that you have to specify that it's :server_notifications seems very redundant, this needs to be changed IMO. I think the SubscribeRequest message should be send with subscribe, and the response would be the state of the rooms (or something else if the type of notifications is metrics etc.)
There was a problem hiding this comment.
I like the current solution. We only send SubscribeRequest once, immediately after starting the notifier. For now I see a scenario, in which we define Notifier inside Application and pass the events we want to receive inside start options.
I don't see a scenario, when we would need to subscribe for some new event late after starting the notifier.
There was a problem hiding this comment.
Whether specyfing :server_notifications is redundant - if Notifier is receiving more than one type of event, we have to specify what kind of events we want the calling process to subscribe to. I think the current way to do it (passing an atom representing certain event type) is the easiest from the user perspective.
There was a problem hiding this comment.
From what I understand, right now starting the Notifier automatically subscribes us to the events passed as the options -- if we keep it this way, we should include a comment about it
At the moment, I feel like it's easy to confuse 1) sending a subscribe request to Jellyfish with 2) subscribing to receive events from the Notifier -- I think we should be more explicit about the distinction.
There was a problem hiding this comment.
As per Łukasz comment, it has been unified now - calling Notifier.subscribe will every time cause sending a SubscribeRequest message to Jellyfish.
lib/jellyfish/notifier.ex
Outdated
| def subscribe(notifier, event_type, room_id) do | ||
| with true <- event_type in @valid_events do |
There was a problem hiding this comment.
| def subscribe(notifier, event_type, room_id) do | |
| with true <- event_type in @valid_events do | |
| def subscribe(notifier, event_type, room_id) when event_type in @valid_types do |
lib/jellyfish/notifier.ex
Outdated
| @doc """ | ||
| Starts the Notifier process and connects to Jellyfish. | ||
|
|
||
| Acts like `start/1` but links to the calling process. | ||
|
|
||
| See `start/1` for more information. | ||
| """ | ||
| @spec start_link(Client.connection_options()) :: {:ok, pid()} | {:error, term()} | ||
| @spec start_link(options()) :: {:ok, pid()} | {:error, term()} |
There was a problem hiding this comment.
Like I said, I think that this function shouldn't need to specify the events.
lib/jellyfish/notifier.ex
Outdated
| @spec subscribe(notifier(), :server_notification, Room.id() | :all) :: | ||
| {:ok, Room.t() | [Room.t()]} | {:error, atom()} | ||
| def subscribe(notifier, event_type, room_id) do |
There was a problem hiding this comment.
Since we'll use this function to subscribe to other event types as well, and since the different requests may or may not contain different options, I'd say we might want to implement a solution like in Jellyfish.Room.add_component -- define separate modules with structs for different event types, as in Jellyfish.Component.{HLS, RTSP}, and allow the user to either pass the struct name (which means we'll use default options) or the struct with custom configuration.
lib/jellyfish/notifier.ex
Outdated
| iex> {:ok, notifier} = Jellyfish.Notifier.start(events: [:server_notifications]) | ||
| {:ok, #PID<0.301.0>} | ||
| iex> {:ok, _rooms} = Jellyfish.Notifier.subscribe(notifier, :all) | ||
|
|
||
| # Subscribe current process to all server notifications. | ||
| iex> {:ok, _rooms} = Jellyfish.Notifier.subscribe(notifier, :server_notifications, :all) |
There was a problem hiding this comment.
From what I understand, right now starting the Notifier automatically subscribes us to the events passed as the options -- if we keep it this way, we should include a comment about it
At the moment, I feel like it's easy to confuse 1) sending a subscribe request to Jellyfish with 2) subscribing to receive events from the Notifier -- I think we should be more explicit about the distinction.
.gitmodules
Outdated
| @@ -1,4 +1,4 @@ | |||
| [submodule "protos"] | |||
| path = protos | |||
| url = https://github.com/jellyfish-dev/protos.git | |||
| branch = master | |||
| branch = actually-subscribe-for-notifications | |||
Codecov Report
@@ Coverage Diff @@
## master #19 +/- ##
==========================================
- Coverage 80.12% 79.26% -0.86%
==========================================
Files 9 9
Lines 166 164 -2
==========================================
- Hits 133 130 -3
- Misses 33 34 +1
Continue to review full report in Codecov by Sentry.
|
Ref: