-
Notifications
You must be signed in to change notification settings - Fork 43
Liveview cohabitation #192
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
Open
guidotripaldi
wants to merge
7
commits into
grych:master
Choose a base branch
from
guidotripaldi:liveview_cohabitation
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
7 commits
Select commit
Hold shift + click to select a range
401b98e
Adds a workaround for cohabitation with LiveView
guidotripaldi 7182602
Better workaround for cohabitation with LiveView
guidotripaldi 96b3171
Removes old commented code
guidotripaldi f21dd64
Updates Travis environment configuration
guidotripaldi 37b89d0
Adds lv_cohabitation test
guidotripaldi 4146652
Updates Travis configuration
guidotripaldi 67414a5
Updates Travis configuration
guidotripaldi File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Oops, something went wrong.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,73 @@ | ||
defmodule DrabTestApp.LVCohabitationTest do | ||
import Drab.Live | ||
use DrabTestApp.IntegrationCase | ||
|
||
# setup do | ||
# index_drab() |> navigate_to() | ||
# # wait for the Drab to initialize | ||
# find_element(:id, "page_loaded_indicator") | ||
# [socket: drab_socket()] | ||
# end | ||
|
||
|
||
describe "LV Cohabitation" do | ||
|
||
# Check Drab page updated by COmmander | ||
test "A Drab page should mutate assigns" do | ||
navigate_to_drab_page() | ||
socket = drab_socket() | ||
|
||
# mutate assign | ||
assert poke(socket, status: "initialised") == {:ok, 1} | ||
|
||
# Process.sleep(500) | ||
|
||
# check mutation | ||
assert peek(socket, :status) == {:ok, "initialised"} | ||
end | ||
|
||
# Check LV page rendered by Controller | ||
test "A LV page rendered trough a Controller should be available" do | ||
navigate_to_lv_page() | ||
assert true | ||
end | ||
|
||
# Check LV page direct render | ||
test "A direct rendered LV page should be available" do | ||
navigate_to_live_page() | ||
assert true | ||
end | ||
end | ||
|
||
# Helpers | ||
|
||
defp navigate_to_drab_page() do | ||
DrabTestApp.Endpoint | ||
|> cohabitation_url(:index_drab, id: 42) | ||
|> navigate_to() | ||
|
||
# # Drab onload is async, so wait a little bitncfor its completionxw | ||
# Process.sleep(500) | ||
|
||
find_element(:id, "page_loaded_indicator") | ||
find_element(:id, "muted_assign_indicator_42") | ||
end | ||
|
||
defp navigate_to_lv_page() do | ||
DrabTestApp.Endpoint | ||
|> cohabitation_url(:index_lv, id: 42) | ||
|> navigate_to() | ||
|
||
find_element(:id, "page_loaded_indicator") | ||
find_element(:id, "muted_assign_indicator_42") | ||
end | ||
|
||
defp navigate_to_live_page() do | ||
DrabTestApp.Endpoint | ||
|> cohabitation_live_url(DrabTestApp.LVCohabitationLive, id: 42) | ||
|> navigate_to() | ||
|
||
find_element(:id, "muted_assign_indicator_42") | ||
end | ||
|
||
end |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,13 @@ | ||
defmodule DrabTestApp.LVCohabitationCommander do | ||
@moduledoc false | ||
|
||
use Drab.Commander | ||
|
||
onload(:page_loaded) | ||
|
||
def page_loaded(socket) do | ||
DrabTestApp.IntegrationCase.add_page_loaded_indicator(socket) | ||
DrabTestApp.IntegrationCase.add_pid(socket) | ||
end | ||
|
||
end |
14 changes: 14 additions & 0 deletions
14
test/support/web/controllers/lv_cohabitation_controller.ex
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,14 @@ | ||
defmodule DrabTestApp.LVCohabitationController do | ||
@moduledoc false | ||
|
||
use DrabTestApp.Web, :controller | ||
require Logger | ||
|
||
def index_drab(conn, params) do | ||
render(conn, "index_drab.html", status: "unititialised", id: params["id"]) | ||
end | ||
|
||
def index_lv(conn, params) do | ||
live_render(conn, DrabTestApp.LVCohabitationLive , session: %{status: "unititialised", id: params["id"]}) | ||
end | ||
end |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For both this and the one in
get_controller_action_name
is there no way to more safely check this without incurring an exception? Exceptions are pretty cheap when not thrown (although setting up the exception handler is not free in any case), but they are much less cheap when they are actually thrown, which would be very common in LiveView cases. If not this looks good as it is. :-)Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't like it either use exceptions in a library but I haven't found a better way, both
Phoenix.Controller.controller_module/1
andPhoenix.Controller.action_name/1
raise if the Controller is unavailable.Any suggestions for some alternatives of course are welcome.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, all it checks is
conn.private[:phoenix_controller]
, similar for the other, which is technically internal and not exposed, although it's never been changed. I wonder if direct access might be warranted, thoughts?Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In fact my first version was similar, it was a check against the presence of
: live_view_module
in the assigns (if !conn.assigns[:live_view_module] do
), but it was a solution too specific for LV, so searching for a a general solution I thought that is better to use the official API because, although unlikely, the parameters for a direct access could be changed tomorrow, and in a library maybe is preferable do not hack too much.Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually I don't know how much heavy thrown exceptions are, it could be interesting to setup some benchmark so to be aware of the actual costs. In any case we could assume that we don't want exceptions management in Drab and use the
conn.private[:phoenix_controller]
hack, and switch to the API based solution if tomorrow things will change.Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, let's test this case:
So in summary, matching via a case extraction is always fastest. The
catch-direct-always-fail
will be the case where Phoenix's functions will lookup and fail (and it always fails for the same reason that phoenix hard-codes in its keys). TheAccess
pattern is thesome_map[some_key]
lookup, it's not great either.In short, a case matcher would be about triple the speed on average. I'm not sure that's really worth caring about then. I'm voting to accept it with the Phoenix calls as it is then. Agree?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the benchmark, interesting results.
I agree, differences are not substantial, and in case we can change our mind anytime