phoenixframework / phoenix_live_reload

Provides live-reload functionality for Phoenix
MIT License
315 stars 90 forks source link

Endpoint Config :path Change Breaks Live Reload Path Match #34

Closed ghost closed 8 years ago

ghost commented 8 years ago

After altering the :path in my phoenix project's config.exs file to:

url: [host: "localhost", path: "/newsite"],

and my endpoint.ex file to:

...
 if code_reloading? do
    socket "/newsite/phoenix/live_reload/socket", Phoenix.LiveReloader.Socket
    plug Phoenix.LiveReloader
    plug Phoenix.CodeReloader
  end
...

The Phoenix.LiveReloader plug's call function will never match on the path info.

i.e. This function definition won't match as long as the path has been prepended:

def call(%Plug.Conn{path_info: ["phoenix", "live_reload", "frame"]} = conn , _) do

I've modified this locally and it looks to be working well. I'll send a PR for your review.

Cheers, Tony

ghost commented 8 years ago

On second thought, I think this isn't a good idea. I can get it working very well for me but this wouldn't be a good modification for the broader community. It can be efficient for one level deep such as prepending /newsite/ to the path but to accommodate anything longer means trying to match the last 3 elements of :path_info with ["phoenix", "live_reload", "frame"] on every request. Then ensure the full path is actually the same as the user defined prefix + /phoenix/live_reload/frame and not something random like "/xyz/phoenix/live_reload/frame".

An example of how this could be handled is here: https://gist.github.com/tesp0/9582e2937047f1f031bcfa100962b086#file-live_reloader-ex-L68-L81

Perhaps it would be a good idea to not grab the endpoint.path() in reload_assets_tag/1. So at least if someone sets their path to something non-standard it won't break live reload. I think that this should just be a static path that is always /phoenix/live_reload/frame.

What are your thoughts?

josevalim commented 8 years ago

@tesp0 thank you for the report. There is a slightly disconnect here. When you use the :path option, it is typically because you are running Phoenix inside a server (like Apache or Nginx) which makes itself think it is at root while it is actually inside "/newsite". So you need to change the URL generation so it knows where it is pointing to.

According to your changes, it seems like you are using /newsite but still effectively accessing it all directly through Phoenix, without the proxying, hence it doesn't work.

Is this interpretation correct or am I missing something?

ghost commented 8 years ago

Thanks for getting back so quick @josevalim :)

Indeed I am trying to run a phoenix site behind a reverse proxy. This is what I'm doing right now after tinkering with it all day:

server {
    listen 8080 default_server;

    location /newsite/phoenix/live_reload/socket/websocket {
        proxy_pass http://localhost:4000;
        proxy_http_version 1.1;
        proxy_set_header Upgrade $http_upgrade;
        proxy_set_header Connection "upgrade";
    }
    location /phoenix {
            proxy_pass http://localhost:4000;
    }
    location /newsite {
        proxy_pass http://localhost:4000;
    }
    location / {
        proxy_pass http://localhost:8090;
    }
}

Oh wait, yes you're completely right José !!!

I should've rewrote the urls with rewrite ^/newsite/(.*)$ /$1 break; and now I understand the purpose of the path option, it makes perfect sense now! I really wish I thought of that before hand :(

I just set it all up correctly and it's working like a dream with original code as well.

Thanks for taking the time, you saved me a lot of time!

Cheers, Tony