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

feat: basepath routing for DevServer vhost emulation #59

Merged
merged 10 commits into from Jan 8, 2024

Conversation

andyl
Copy link
Contributor

@andyl andyl commented Jan 5, 2024

This is a quick working solution to #57 - it allows the site developer to configure a :base_path attribute in config.exs.

The base_path attribute is used in router.ex to adjust the static path, and in Mix.Tasks.Tableau.Server to adjust the logger message.

If the :base_path is not set in config.exs, the system behaves just as it does now.

For my use case as described in #57, this solution works great!

If you reject this PR in favor of a better solution, no problemo. If you want docs, or tests etc. I will supply. Just putting this out there for ideas and feedback.

Comment on lines 22 to 25
case Application.get_env(:tableau, :config)[:base_path] do
"" -> ""
path -> "/" <> path
end
Copy link
Collaborator

Choose a reason for hiding this comment

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

You can use Path.join/2 here

@@ -14,7 +16,7 @@ defmodule Tableau.Router do
plug :rerender

plug Tableau.IndexHtml
plug Plug.Static, at: "/", from: "_site", cache_control_for_etags: "no-cache"
plug Plug.Static, at: "/#{basepath}", from: "_site", cache_control_for_etags: "no-cache"
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would also use Path.join/2 here as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I used Path.join/2, and also fixed the warning associated with Application.get_env/2.

@@ -5,6 +5,7 @@ defmodule Tableau.Config do

defstruct [
:url,
base_path: "",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please add documentation for this. Its in the Tableau moduledoc

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@andyl
Copy link
Contributor Author

andyl commented Jan 8, 2024

I added Path.join/2, fixed a warning with Application.get_env/2 and added base_path moduledocs to tableau.ex.

Comment on lines 22 to 25
case Application.get_env(:tableau, :config)[:base_path] do
"" -> ""
path -> Path.join("/", path)
end
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
case Application.get_env(:tableau, :config)[:base_path] do
"" -> ""
path -> Path.join("/", path)
end
Path.join("/", Application.get_env(:tableau, :config)[:base_path])

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

lib/tableau.ex Outdated
@@ -4,9 +4,69 @@ defmodule Tableau do

* `:include_dir` - string - Directory that is just copied to the output directory. Defaults to `extra`.
* `:timezone` - string - Timezone to use when parsing date times. Defaults to `Etc/UTC`.
* `:base_path - string - base path to use with Github Pages or web-servers that use a location prefix for Vhosts
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
* `:base_path - string - base path to use with Github Pages or web-servers that use a location prefix for Vhosts
* `:base_path - string - Development server root . Defaults to `/`.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

lib/tableau.ex Outdated
Comment on lines 11 to 69

## Working with Github Pages or web-server Vhosts

Github Pages provides a root url like `https://andyl.github.io/xmeyers`, where
the `xmeyers` prefix is a virtual host identifier tied to the repo at
`https://github.com/andyl/xmeyers`.

With Nginx and Apache it is common to use a location prefix for Vhosts.
Here is an example NGINX config snippet:

```
server {
listen 80;
server_name myhost.com;

location /site1 {
# Configuration for site1 - eg the root directive to a directory
# root /var/www/site1;
}

location /site2 {
# Configuration for site2
# Similar configuration as site1, adjusted for site2 specifics
}

# Other configuration...
}
```

To make Tableau's development server (`mix tableau.server`) also use a Vost
prefix, configure your app with the `:base_path` attribute. With that, your
website HREFs will work in both development and production.

## Example

In `config/config.exs`:

```elixir
config :tableau, :config,
url: "http://localhost:4999",
base_path: "xmeyers",
markdown: [
...
]
```

In `root_layout.ex`:

```elixir
def template(assigns) do
~H\"""
<!DOCTYPE html>
<html>
<head>
<title>Xmeyers</title>
<link rel="icon" href="/xmeyers/static/img/favicon.ico" type="image/x-icon" />
<link rel="stylesheet" type="text/css" href="/xmeyers/css/site.css" />
...
```
Copy link
Collaborator

Choose a reason for hiding this comment

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

Delete all of this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@@ -6,6 +6,8 @@ defmodule Tableau.Router do

require Logger

@base_path Path.join("/", Application.compile_env(:tableau, :config)[:base_path] || "")
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think you actually need the || "" since it defaults to ""

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In my testing it defaults to nil, which causes Path.join/2 to raise an error.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh, sorry, I was thinking of the default coming from the struct. This is fine then, should also add to the other spot instead of that case expression

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I took a closer look at this. Turns out there is a small difference in the arguments accepted by Application.get_env and Application.compile_env. I believe the forms I used are correct, and tested it with and without a base_path configuration.

@andyl
Copy link
Contributor Author

andyl commented Jan 8, 2024

All changes done.

@mhanberg mhanberg merged commit 641cb20 into elixir-tools:main Jan 8, 2024
8 checks passed
@mhanberg
Copy link
Collaborator

mhanberg commented Jan 8, 2024

awesome work, thanks!

@andyl andyl deleted the basepath_routing branch January 8, 2024 16:01
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.

None yet

2 participants