-
Notifications
You must be signed in to change notification settings - Fork 107
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
feat: always generate site endpoints #740
Changes from all commits
302fcee
0046e89
6689ab5
45b28c9
53b6a13
9de3a74
b7f8c97
00c650b
5b736e4
143fcc7
f358b8c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -42,9 +42,19 @@ Demo.Repo.start_link() | |
Ecto.Migrator.run(Demo.Repo, path, :up, all: true, log_migrations_sql: true) | ||
Demo.Repo.stop() | ||
|
||
Application.put_env(:beacon, DemoWeb.Endpoint, | ||
Application.put_env(:beacon, DemoWeb.ProxyEndpoint, | ||
adapter: Bandit.PhoenixAdapter, | ||
check_origin: {DemoWeb.ProxyEndpoint, :check_origin, []}, | ||
url: [port: 4001, scheme: "http"], | ||
http: [ip: {0, 0, 0, 0}, port: 4001], | ||
live_view: [signing_salt: "aaaaaaaa"], | ||
secret_key_base: String.duplicate("a", 64), | ||
server: true | ||
) | ||
|
||
Application.put_env(:beacon, DemoWeb.Endpoint, | ||
adapter: Bandit.PhoenixAdapter, | ||
http: [ip: {0, 0, 0, 0}, port: 4100], | ||
server: true, | ||
live_view: [signing_salt: "aaaaaaaa"], | ||
secret_key_base: String.duplicate("a", 64), | ||
|
@@ -87,12 +97,21 @@ defmodule DemoWeb.Router do | |
end | ||
end | ||
|
||
defmodule DemoWeb.ProxyEndpoint do | ||
use Beacon.ProxyEndpoint, | ||
otp_app: :beacon, | ||
session_options: [store: :cookie, key: "_beacon_dev_key", signing_salt: "pMQYsz0UKEnwxJnQrVwovkBAKvU3MiuL"], | ||
fallback: DemoWeb.Endpoint | ||
end | ||
|
||
defmodule DemoWeb.Endpoint do | ||
use Phoenix.Endpoint, otp_app: :beacon | ||
|
||
@session_options [store: :cookie, key: "_beacon_dev_key", signing_salt: "pMQYsz0UKEnwxJnQrVwovkBAKvU3MiuL"] | ||
|
||
socket "/live", Phoenix.LiveView.Socket, websocket: [connect_info: [session: @session_options]] | ||
# TODO: add this function in the `gen.site` generator | ||
def proxy_endpoint, do: DemoWeb.ProxyEndpoint | ||
Comment on lines
+112
to
+113
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If this is supposed to represent the "default" Endpoint that Phoenix gives you, then this should go in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's the other way around actually. From the site endpoint we need to know the proxy endpoint in order to fetch the scheme and port to generate URLs. I did include that function mostly to avoid adding a new required option in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think the confusion was, I thought this was supposed to represent the "default" phoenix endpoint, and not a generated beacon endpoint. So my comment is no longer relevant 🙂 |
||
|
||
socket "/phoenix/live_reload/socket", Phoenix.LiveReloader.Socket | ||
|
||
plug Phoenix.LiveReloader | ||
|
@@ -1246,7 +1265,8 @@ Task.start(fn -> | |
Demo.Repo, | ||
{Phoenix.PubSub, [name: Demo.PubSub]}, | ||
{Beacon, sites: [dev_site, dy_site]}, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. should these sites get their own endpoint now instead of using There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ideally yes to simulate the same config used in a real app. I created the Proxy endpoint but forgot to create the site endpoints. |
||
DemoWeb.Endpoint | ||
DemoWeb.Endpoint, | ||
DemoWeb.ProxyEndpoint | ||
] | ||
|
||
{:ok, _} = Supervisor.start_link(children, strategy: :one_for_one) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -185,6 +185,9 @@ defmodule Beacon.Router do | |
Then Beacon will reference both of those sitemaps in the top-level index: | ||
* `my_domain.com/sitemap_index.xml` | ||
|
||
Note that `beacon_sitemap_index` will include the sitemap URL of all mounted sites | ||
in the router, so that macro should be at the root and not duplicated. | ||
|
||
Comment on lines
+188
to
+190
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. what does it mean that it should be at the root? is it still OK for e.g. scope "/admin", as: :beacon_live_admin do
pipe_through [:admin_browser, ...]
get "/session/sign_out", SessionController, :sign_out
beacon_live_admin "/"
end
scope "/" do
pipe_through [:browser, ...]
beacon_site "/", site: :dockyard_com
end There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe just bad wording here (that can be improved). Since the sitemap index is per-host, then there's no need to duplicate it, thus it makes more sense to include it in a "root scope" instead of a nested scope. That's also related to the hierarchy that's described in the existing docs. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ok just checking there wasn't an additional meaning that needed documentation There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Wait... giving a second thought we can't render all sites in the sitemap index. Giving 2 different scopes on different There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. that would mean multiple sitemap indexes, right? we don't currently have a way to do that 🤔 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Eg:
Should foo.com/sitemap_index.xml include site_c? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think in order to properly implement this, we need some data model "in-between"
where that There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. it's the same issue with There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
So either an option like:
Where the default could be "all" sites. Or introspect the router to accumulate sites per scope, similar to how we accumulate sites in Although that's not responsibility of this PR. |
||
## Requirements | ||
|
||
Note that your sitemap index cannot have a path which is "deeper" in the directory structure than | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,6 +1,6 @@ | ||
<?xml version="1.0" encoding="UTF-8"?> | ||
<sitemapindex xmlns="http://www.sitemaps.org/schemas/sitemap/0.9"> | ||
<%= for url <- @urls do %><sitemap> | ||
<sitemapindex xmlns="http://www.sitemaps.org/schemas/sitemap/0.9"><%= for url <- @urls do %> | ||
<sitemap> | ||
Comment on lines
-2
to
+3
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. good catch |
||
<loc><%= url %></loc> | ||
</sitemap><% end %> | ||
</sitemapindex> |
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.
with this removed from the existing Endpoint, will this break pages outside of beacon? can the ProxyEndpoint work for non-beacon pages?
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 endpoint is supposed to be one of the site endpoints, probably should rename it to
DevEndooint
and create a newDyEndpoint
. No need to have an "app endooint" in dev.exs tho because we wouldn't use it for anything.