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

Adds support for allowing PubSub subscription and events handling #189

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 31 additions & 0 deletions lib/drab.ex
Original file line number Diff line number Diff line change
Expand Up @@ -213,6 +213,15 @@ defmodule Drab do
{:noreply, state}
end

# pass any other handle_info to the Commander `handle_info_message/2` callbacks,
# which expect `message` and `socket` params (no `state` reply).
def handle_info(message, state) do
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure this should be in Drab, it should probably be in Drab.Channel, honestly Drab shouldn't be a genserver at all, everything it does and all the information it holds really should be in Drab.Channel, but that's a refactor for another day. ^.^;

For note, Drab.Channel already handles phoenix messages, although it passes them to the client for bouncing back 'through' drab, that should probably be restricted to a specific set of 'types' of topics for such javascript bouncing, so normal phoenix events can be handled differently. Also, Drab.Commander has subscribe and unsubscribe calls for registering to phoenix pubsub topics through the endpoint via Drab.Channel. I wonder what messages it handles through here actually, I.E. I wonder what the purpose of it is to send only to the JS side, hmm... This needs to be researched. Maybe Drab already has this functionality but it could just be 'fleshed out' a little bit...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe Drab already has this functionality but it could just be 'fleshed out' a little bit...

maybe, for now I haven't found a better way to support PubSub natively in a Commander

state.socket
|> get_commander()
|> apply(:handle_info_message, [message, state.socket])
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is synchronous! Definitely shouldn't be! Drab has async calls in this module, should follow the same pattern as them.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, why is this name of handle_info_message hardcoded? Although I like hardcoded names, that's not Drab's way.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it is hardcoded as well as GenServer name handle_info/2' callback is. I've choose not to call it just "handle_info" because though its role is similar, it is not identical (because it receive a socketinstead of astate, and doesn't return a state`. Naming is difficult, so certainly we can choose another better name (actually I don't like too much 'handle_info_message' but after thinking a lot I couldn't find a better one). It have to be hardcoded because there is no other way, as far as I know, to have many of this kind of handlers (each one with it own pattern matching on message), in a Commander.

{:noreply, state}
end

@doc false
@spec handle_cast(tuple, t) :: {:noreply, t}
def handle_cast({:onconnect, socket, payload}, %Drab{commander: commander} = state) do
Expand Down Expand Up @@ -240,10 +249,18 @@ defmodule Drab do
def handle_cast({:onload, socket, payload}, %Drab{commander: commander} = state) do
socket = transform_socket(payload["payload"], socket, state)

# onload_init sync call with arity/0
handle_callback_sync(commander, commander_config(commander).onload_init)

# onload async call
onload = commander_config(commander).onload
handle_callback(socket, commander, onload)

for shared_commander <- state.commanders do
# onload_init sync call with arity/0
handle_callback_sync(commander, commander_config(commander).onload_init)

# onload async call
onload = commander_config(shared_commander).onload
handle_callback(socket, shared_commander, onload)
end
Expand Down Expand Up @@ -277,6 +294,7 @@ defmodule Drab do
end
end)

# Handles callbacks asynchronously
@spec handle_callback(Phoenix.Socket.t(), atom, atom) :: Phoenix.Socket.t()
defp handle_callback(socket, commander, callback) do
if callback do
Expand All @@ -294,6 +312,19 @@ defmodule Drab do
socket
end

# Handles callbacks synchronously
# Temporary disabled until it will be needed, for avoiding warning
# @spec handle_callback_sync(Phoenix.Socket.t(), atom, atom) :: Phoenix.Socket.t()
# defp handle_callback_sync(socket, commander, callback) do
# if callback, do: apply(commander, callback, [socket])
# socket
# end

@spec handle_callback_sync(atom, atom) :: any
defp handle_callback_sync(commander, callback) do
if callback, do: apply(commander, callback, [])
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This really shouldn't exist, like really really shouldn't exist.

If someone wants the pid to drab they can just call the Drab.pid(socket) as normal in the onload callback (or any other callback). The presence of this function makes it easy to accidentally perform complex operations and implies they can only get the pid via this function instead of just using Drab.pid(socket) as normal.

Copy link
Contributor Author

@guidotripaldi guidotripaldi Aug 7, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is necessary to let to subscribe to PubSub in the Commander. This because it is deprecated to pass a PID to the PubSub.subscribe function, otherwise we could just use onload event and pass the Drab.pid(socket) to PubSub. This is the reason behind the choice to have a new sync handler. I don't really like it either, but afaik there is no other ways to implement a pubsub subscription from inside a Commander.

end

@spec transform_payload(map, t) :: map
defp transform_payload(payload, state) do
all_modules = DrabModule.all_modules_for(state.commander.__drab__().modules)
Expand Down
110 changes: 109 additions & 1 deletion lib/drab/commander.ex
Original file line number Diff line number Diff line change
Expand Up @@ -132,13 +132,22 @@ defmodule Drab.Commander do
defmodule DrabExample.PageCommander do
use Drab.Commander

onload_init: do_init
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should not exist, it is sync.

onload :page_loaded
onconnect :connected
ondisconnect :disconnected

before_handler :check_status
after_handler :clean_up, only: [:perform_long_process]

def do_init do
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto, no sync calls.

# notice that this callback is called synchronously, so
# it is not suitable for longer operations that could
# slow down the main process.
# Any operation here inherits the main process PID
...
end

def page_loaded(socket) do
...
end
Expand Down Expand Up @@ -169,6 +178,14 @@ defmodule Drab.Commander do
Launched every time client browser connects to the server, including reconnects after server
crash, network broken etc

#### `onload_init`
Launched synchronously just before the `onload` callback. Unlike all the others callbacks
that runs in their own process, this callback runs in the main process, and therefore is useful
to call those operations who needs the main process PID to operate correctly,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No it is not useful for that because you can just Drab.pid(socket) to get the pid, and this one works in any callback, not just on init.

as e.g. the subscription to PubSub events.
This callback is not suitable for hosting longer or harmful operations, since this
could slow down or broke the main process. Use with caution.

#### `onload`
Launched only once after page loaded and connects to the server - exactly the same like
`onconnect`, but launches only once, not after every reconnect
Expand All @@ -191,6 +208,11 @@ defmodule Drab.Commander do
Runs after the event handler. Gets return value of the event handler function as a third argument.
Can be filtered by `:only` or `:except` options, analogically to `before_handler`


### `handle_info_message`
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems to focus a lot on 'pubsub' when it is nothing of the sort, only thing it does is pass in otherwise unhandled messages, I.E it's an info message handler, no need to focus on pubsub stuff and just say that when an unknown message is sent to the drab pid then it is received here. Phoenix.PubSub already clarifies how Phoenix.PubSub works.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes it is focused on PubSub because this is the main and most useful reason to add this stuff to Drab, but of course we change the documentation.

Launched every time a GenServer/PubSub event to which you have subscribed is fired.
See "PubSub support" for more informations.

### Using callbacks to check user permissions
Callbacks are handy for security. You may retrieve controller name and action name from the
socket with `controller/1` and `action/1`.
Expand Down Expand Up @@ -240,6 +262,92 @@ defmodule Drab.Commander do
Drab injects function `render_to_string/2` into your Commander. It is a shorthand for
`Phoenix.View.render_to_string/3` - Drab automatically chooses the current View.

## PubSub support
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Technically Drab already has PubSub support as of https://github.com/grych/drab/releases/tag/v0.8.3 although not of the form you are expecting, rather it passes messages to the javascript to then be handled there. Messages to the Drab process should already be web-ready regardless, unknown pubsub messages should never reach drab regardless.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems that Drab hasn't any kind of PubSub support. It just use PubSub for its internal uses, that it is a different things to let the developer uses Phoenix.PubSub directly inside a live page if needed, as for example LiveView does and suggest.


You can use the PubSub mechanism to automatically update your pages on data changes.

Suppose for example you have a backend module that interfaces the database, and you
whishes to automatically update your page every time a change occours in a
database table by some operations made in other part of your application. You can
implement

Suppose you need to automatically update your page every time a change occours in a
database table by some operations made in other part of your application. You can
implement a PubSub mechanism to react from the commander to any events you have
subscribed to: add the subscription to the desidered `PubSub` in the
`onload_init` event handler, then add a `handle_info_message/2` handler for any
event you need to react to.

Please note that the `handle_info_message/2` handler is a simplified version of the
GenServer/PubSub `handle_info/2` handler, it expects a message parameter, identical
to the standard `handle_info/2` one, and a `socket` parameter, instead of the
`state` parameters, and it doesn't have to return anything.

For example:

# the backend:
defmodule MyApp.Backend do
# PubSub stuff
@topic inspect(__MODULE__)

def subscribe() do
Phoenix.PubSub.subscribe(MyApp.PubSub, @topic)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't taking a PID, that's very weird...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is not possible to pass the pid to a PubSub.subscription function. Otherwise we could just call Phoenix.PubSub.subscribe(MyApp.PubSub, Drab.pid(socket), @topic) in the onload handler, without any need of the added sync call.

This because, although documented, Phoenix.PubSub.subscribe/3 is deprecated. If used it works for now, but warnings with: "Passing a Pid to Phoenix.PubSub.subscribe is deprecated. Only the calling process may subscribe to topics" so we cannot anymore use it.

This is the exact reason because I've elaborate a solution through the onload_init stuff.

Copy link
Collaborator

@OvermindDL1 OvermindDL1 Aug 7, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is not possible to pass the pid to a PubSub.subscription function.

https://hexdocs.pm/phoenix_pubsub/Phoenix.PubSub.html#subscribe/3
Why not? It's not marked as deprecated in the docs. Nor does it have a @deprecated annotation.

Besides, Drab already has a method to subscribe to pubsub topics via it's own subscribe function, it just needs to detect drab specific things and 'other' things then route the others back to the commands instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

end

defp notify_subscribers({:ok, result}, event) do
Phoenix.PubSub.broadcast(MyApp.PubSub, @topic, {__MODULE__, event, result})
{:ok, result}
end
defp notify_subscribers({:error, reason}, _event), do: {:error, reason}

# CRUD stuff
...
def create(...) do
...
|> Repo.insert()
|> notify_subscribers([:data, :created])
end

def update(...) do
...
|> Repo.update()
|> notify_subscribers([:data, :updated])
end

def delete(...) do
...
|> Repo.delete()
|> notify_subscribers([:data, :deleted])
end
end

# the Commander
defmodule MyApp.Commander do
use Drab.Commander

onload_init(:do_init)

def do_init() do
# subscribe to any desidered PubSub
MyApp.Backend.subscribe()
end

...

# handles the events
def handle_info_message({MyApp.Backend, [:data, :created], result}, socket) do
poke(socket, data: result)
end

# handles the events
def handle_info_message({MyApp.Backend, [:data, :updated], result}, socket) do
poke(socket, data: result)
end

...
end


### Examples:

buttons = render_to_string("waiter_example.html", [])
Expand Down Expand Up @@ -379,7 +487,7 @@ defmodule Drab.Commander do
end
end

Enum.each([:onload, :onconnect, :ondisconnect], fn macro_name ->
Enum.each([:onload_init, :onload, :onconnect, :ondisconnect], fn macro_name ->
@doc """
Sets up the callback for #{macro_name}. Receives handler function name as an atom.

Expand Down
1 change: 1 addition & 0 deletions lib/drab/commander/config.ex
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ defmodule Drab.Commander.Config do
defstruct commander: nil,
controller: nil,
view: nil,
onload_init: nil,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a synchronous call, should not exist.

onload: nil,
onconnect: nil,
ondisconnect: nil,
Expand Down
33 changes: 33 additions & 0 deletions test/integration/pubsub_test.exs
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
defmodule DrabTestApp.PubsubTest do
import Drab.Live
use DrabTestApp.IntegrationCase

defp pubsub_index do
pubsub_url(DrabTestApp.Endpoint, :index)
end

setup do
pubsub_index() |> navigate_to()
# wait for the Drab to initialize
find_element(:id, "page_loaded_indicator")
[socket: drab_socket()]
end

describe "PubSub" do
test "Subscribed PubSub events should trigger `handle_info_message` in the Commander" do
socket = drab_socket()

# Changes some data in the db, this will trigger a PubSub event
DrabTestApp.Backend.append_element(42)

# Above operation is async, so wait a little bit
# for its completion before checking the result
Process.sleep(500)

# Check if the `handle_info_message` had changed the
# page assigns according to the new data
assert peek(socket, :status) == {:ok, "updated"}
assert peek(socket, :data) == {:ok, [1, 2, 3, 42]}
end
end
end
31 changes: 31 additions & 0 deletions test/support/lib/backend.ex
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
defmodule DrabTestApp.Backend do
@moduledoc false

# PubSub support
@topic inspect(__MODULE__)

def subscribe() do
Phoenix.PubSub.subscribe(DrabTestApp.PubSub, @topic)
end

defp notify_subscribers({:ok, result}, event) do
Phoenix.PubSub.broadcast(DrabTestApp.PubSub, @topic, {__MODULE__, event, result})
{:ok, result}
end

defp notify_subscribers({:error, reason}, _event), do: {:error, reason}

# Fake db access
@fake_db [1, 2, 3]

def get_data() do
@fake_db
end

def append_element(element) do
get_data()
|> List.insert_at(-1, element)
|> (&({:ok, &1})).()
|> notify_subscribers([:data, :updated])
end
end
2 changes: 2 additions & 0 deletions test/support/router.ex
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,8 @@ defmodule DrabTestApp.Router do
get("/tests/live/broadcasting", LiveController, :broadcasting, as: :broadcasting)

get("/tests/element", ElementController, :index, as: :element)

get("/tests/pubsub", PubsubController, :index, as: :pubsub)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Drab already has pubsub functionality, although it's tested in the LiveController test. It really should be pulled out into it's own test, and Drab.Channel's message receiver should be changed so if the topic starts with drab: then it should do as it does now, else it should call a callback in the appropriate commander (which could potentially be shared commanders).

Copy link
Contributor Author

@guidotripaldi guidotripaldi Aug 7, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand this comment. Here we are just testing the new added functionalities with a PubSub lifecycle case, thus the name 'pubsub' for this test.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh I was just saying that the little bit of pubsub use that drab uses is testing in the /tests/live/broadcasting route, when they should probably be pulled out into their own. ^.^

end

# Other scopes may use custom stacks.
Expand Down
21 changes: 21 additions & 0 deletions test/support/web/commanders/pubsub_commander.ex
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
defmodule DrabTestApp.PubsubCommander do
@moduledoc false

use Drab.Commander

onload_init(:do_init)
onload(:page_loaded)

def do_init() do
DrabTestApp.Backend.subscribe()
end

def page_loaded(socket) do
DrabTestApp.IntegrationCase.add_page_loaded_indicator(socket)
DrabTestApp.IntegrationCase.add_pid(socket)
end

def handle_info_message({DrabTestApp.Backend, [:data, :updated], result}, socket) do
poke(socket, status: "updated", data: result )
end
end
2 changes: 1 addition & 1 deletion test/support/web/controllers/element_controller.ex
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,6 @@ defmodule DrabTestApp.ElementController do
require Logger

def index(conn, _params) do
render(conn, "index.html")
render(conn, "index.html")
end
end
10 changes: 10 additions & 0 deletions test/support/web/controllers/pubsub_controller.ex
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
defmodule DrabTestApp.PubsubController do
@moduledoc false

use DrabTestApp.Web, :controller
require Logger

def index(conn, _params) do
render(conn, "index.html", status: "initilised", data: DrabTestApp.Backend.get_data())
end
end
6 changes: 6 additions & 0 deletions test/support/web/templates/pubsub/index.html.drab
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
<h3 id="header">Drab Tests</h3>

<div id="begin"></div>
<div id="drab_pid"></div>

<div>status_: <%= @status %> data: <%= inspect @data %></div>
5 changes: 5 additions & 0 deletions test/support/web/views/pubsub_view.ex
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
defmodule DrabTestApp.PubsubView do
@moduledoc false

use DrabTestApp.Web, :view
end