-
Notifications
You must be signed in to change notification settings - Fork 182
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
Update phoenix to 1.5.x #460
Conversation
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 can help dig into specifics you run into in "things to verify" but this looks good to me. A few notes, mostly for my own understanding.
Re "I do not know how to verify that I have migrated the PubSub configuration correctly".... Not 100% sure but I think if presence is working correctly we are all set. I would imagine things would break very noticeably if there were issues.
@@ -34,7 +34,7 @@ defmodule RetWeb do | |||
namespace: RetWeb | |||
|
|||
# Import convenience functions from controllers | |||
import Phoenix.Controller, only: [get_flash: 2, view_module: 1] | |||
import Phoenix.Controller, only: [get_flash: 2] |
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 see this is called out specifically to check in the TODOs but I don't see any mention of this in the upgrade notes. Whats this change do?
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.
This change makes it so that Phoenix.Controller.view_module/1
is not automatically accessible from all the views (modules with use RetWeb, :view
).
First I made the changes as per this line source:
[Layout] Use <%= @inner_content %> instead of <%= render @view_module, @view_template, assigns %> for rendering the child layout
Then I noticed that view_module
was never used anywhere else in the project. I figured it was safe (and maybe desirable) for us to remove it, but I don't feel strongly about it.
@@ -18,7 +18,7 @@ defmodule RetWeb.ChannelCase do | |||
using do | |||
quote do | |||
# Import conveniences for testing with channels | |||
use Phoenix.ChannelTest | |||
import Phoenix.ChannelTest |
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.
The upgrade guide says to replace use Phoenix.ConnTest
with
import Plug.Conn
import Phoenix.ConnTest
Do we need to import Conn as well?
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 think so -- the note for ChannelTest
only suggests replacing use
with import
:
[ChannelTest] use Phoenix.ChannelTest is deprecated in favor of import Phoenix.ChannelTest
[ConnTest] use Phoenix.ConnTest is deprecated in favor of import Plug.Conn; import Phoenix.ConnTest
Superseded by #668 |
Upgrade Notes
Preparing to update
I wrote some of the reasons I want to update in #457
I made sure phoenix dependencies were up to date before updating anything else by running
mix deps.update phoenix
The results are in #458
Updating to phoenix 1.5
I followed the phx-1.5-upgrade guide.
I updated the phx.new project generator. (I believe everyone will need to do this.)
I tried to bump
:phoenix
,phoenix_pubsub
, andplug_cowboy
.I cannot bump
phoenix_pubsub
to 2.0.x without also updatingabsinthe_phoenix
. Since I already started this in another branch, I merged that change into my primary branch and then proceeded to updateabsinthe_phoenix
to 2.0.0 .The
absinthe_phoenix
Changelog does not seem to have any changes that we need to worry about.The
phoenix_pubsub
Changelog does not have any changes we need to handle directly. Instead, we need to make some changes to our use of thePhoenix.PubSub
API as described in thephoenix
1.5 upgrade guide.I do not know how to verify that I have migrated the PubSub configuration correctly (in prod.exs, config.exs, and application.ex) as described in the guide. Help with this would be appreciated.
The
plug_cowboy
Changelog does not have any changes we need to worry about. A benefit we will get is that telemetry events have been added viacowboy_telemetry
and are enabled by default.Updating
plug_cowboy
requires updatingcowboy
to at least 2.7. The migration guide does not appear to have any changes we need to worry about. Cowboy requires Erlang/OTP 20.0+, which is a requirement we satisfy (we use Erlang 22.) Cowboy’s dependencies (ranch
andcowlib
) are used exclusively bycowboy
, so their updates are not concerning to us. Previously, we downgraded away fromcowboy 2.7.0
because of a bug with multipart uploads. However, this appears to be fixed so I will go ahead and allow it to update. Cowboy is only in our `mix.exs` file in the first place because we wanted to bump it to version 2.7. However, it is not a direct dependency so I removed it from mix.exs.Things to verify
@inner_content
)view_module: 2
import fromret_web.ex
did not break anything.RetWeb.ConnCase
https://gist.github.com/chrismccord/e53e79ef8b34adf5d8122a47db44d22f#update-your-testsphoenix 1.5
The admin interface is returning 502 gateway errors. I don’t know why. I thought maybe updatingThis was only a problem with my local dev environment, and I fixed it.reverse_proxy_plug
would fix the issue, but it seemed to have no effect.Related PR’s and issues
phoenix_live_dashboard
in a separate PR built off this branch