Skip to content

Release compat #26

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
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

bcardarella
Copy link

This PR allows for running mix release as the LiveReload code is now guarded against if it is explicitly opted out of

In addition, the plug option from start/1 was moved into start_link/1 so the Router can be overridden when using start_link/1 directly

Added an example to run for using start_link/1 in a Supervisor. This example stack traces prior to this PR

Also updated to latest phoenix_live_view

This PR allows for running `mix release` as the LiveReload
code is now guarded against if it is explicitly opted out of

In addition, the `plug` option from `start/1` was moved into `start_link/1`
so the Router can be overridden when using `start_link/1` directly
{PhoenixPlayground, plug: plug, live_reload: false}
]

opts = [strategy: :one_for_one, name: __MODULE__]
Copy link
Contributor

Choose a reason for hiding this comment

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

__MODULE__ expands to nil in scripts:

Suggested change
opts = [strategy: :one_for_one, name: __MODULE__]
opts = [strategy: :one_for_one]

]

opts = [strategy: :one_for_one, name: __MODULE__]
Supervisor.start_link(children, opts)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Supervisor.start_link(children, opts)
{:ok, _} = Supervisor.start_link(children, opts)


opts = [strategy: :one_for_one, name: __MODULE__]
Supervisor.start_link(children, opts)

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
System.no_halt(true)

plug :run_live_reload

defp run_live_reload(conn, _options) do
if Application.get_env(:phoenix_playground, :live_reload) do
Copy link
Contributor

Choose a reason for hiding this comment

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

we already have a check if live_reload is enabled on line 22, I'm curious if you intentionally left this check here to allow changing the value at runtime somehow?

Copy link
Author

Choose a reason for hiding this comment

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

I'll go through this, it was early on and I may have spiked this incorrectly.

@wojtekmach
Copy link
Contributor

This PR allows for running mix release as the LiveReload code is now guarded against if it is explicitly opted out of

WDYT about adding an examples/demo_release/, a minimal Mix project that showcases the release capability?

@bcardarella
Copy link
Author

@wojtekmach that makes sense, I can add those in this week.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants