Warn if no <body> present

wodow commented 10 months ago

As noted by @ellismarkf in https://github.com/phoenixframework/phoenix_live_reload/issues/134#issuecomment-1471167134 , this library requires <body> to be present in root.html.heex for the browser to refresh following a detected code change and subsequent reload.

It would be useful to warn if <body> is not present due to e.g. developer shenangians with the template structure.

wodow commented 10 months ago

Proposal: <body> is checked for within before_send_inject_reloader/3 , so we could log a warning in the else case of the check.

josevalim commented 10 months ago

Please do submit a PR!

kuon commented 8 months ago

Ok, I got bitten by this, so I tried implementing a PR, but it is more subtle that I anticipated. In before_send_inject_reloader/3 we get the full response, with a <body> tag even if it was added elsewhere that in the root layout.

The problem, is, I think, that when live view kicks in, it re-renders everything, including the app layout, this re-render removes the injected live reload tags. That is why the body tag needs to be in the root layout which is untouched by the live view.

So to warn the user, we have to get the root layout in before_send_inject_reloader/3, read the file and check for <body>.

It is not hard to implement, but it adds a file read on each request.

Something like:

        layout = Phoenix.Controller.root_layout(conn)
        if not layout_contains_body_tag?(layout) do
          IO.warn(
            "Your root template does not contain a <body> tag. " <>
            "The <body> tag is required for the live reload plug to work"
          )
        end

and layout_contains_body_tag? would need to find the template file and read it and search for <body> in it.

The other approach would be to inject some JS in the <head> tag, something like setInterval() that checks every second if the live reload tag is present in the page, a simple query selector would work, and this would console.error() on the browser side.

Last but not least, add a comment in the root.html.heex template when doing phx.new, add same comment in the dev.exs config above live reload section, as those are the two places where we look in case of issues. Something like: # If using live view, ensure your <body> tag is located in your root template.

Let me know what you think.

wodow commented 8 months ago

Thanks for looking into this, @kuon !

Are you saying that has_body?/1 at https://github.com/phoenixframework/phoenix_live_reload/blob/e8a8d800e63c34fabb217af480b742af78a35872/lib/phoenix_live_reload/live_reloader.ex#L157 always returns true then? Arguably that would be a bug in its own right.

add a comment in the root.html.heex ...

That's a good stopgap IMHO.

kuon commented 8 months ago

Yes has_body? always returns true, because it is called on the fully rendered page, so the html actually have a body, even if this body is not within the root template.

I also think the comment is enough.

chrismccord commented 8 months ago

We can't guarantee that the user has a root layout, or a root layout file, so that unfortunately is not going to work.

kuon commented 8 months ago

We can't guarantee that the user has a root layout, or a root layout file, so that unfortunately is not going to work.

We could just add a comment in the generated dev.exs, like this:

# Note: The browser reloading code will not work if the 
# <body> tag is rendered with live view. In typical situation
# it means to keep the <body> inside your root layout.
config :book, BookWeb.Endpoint,
  live_reload: [
    patterns: [
      ~r"priv/static/.*(js|css|png|jpeg|jpg|gif|svg)$",
      ~r"priv/gettext/.*(po)$",
      ~r"lib/book_web/.*/.*(ex)$"
    ]
  ]

Another idea would be to add an attribute that live view would understand, similar to `phx-update="ignore"`` but stronger, it would tell live view to keep the tag even if the parent is removed, then the generated tag from this library would include it and be protected. But I don't know how feasible and desired this would be.

chrismccord commented 8 months ago

A LV wrapping the entire body is a pretty large edgecase that I'm not overly worried about. A better way to handle this would be to open a public function on the LV side (like making the current private reload_assets_tag public with a better name) and having folks call that in their live layout to embed the LR iframe.

phoenixframework / phoenix_live_reload

Warn if no <body> present #140