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.

Sign up for GitHub

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

Jump to bottom

Warn if no <body> present #140

Open
wodow opened this issue Nov 1, 2023 · 8 comments
Open

Warn if no <body> present #140

wodow opened this issue Nov 1, 2023 · 8 comments

Comments

@wodow
Copy link

wodow commented Nov 1, 2023

As noted by @ellismarkf in #134 (comment) , 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
Copy link
Author

wodow commented Nov 1, 2023 •
edited

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
Copy link
Member

Please do submit a PR!

@kuon
Copy link

kuon commented Dec 29, 2023

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
Copy link
Author

wodow commented Jan 8, 2024

Thanks for looking into this, @kuon !

Are you saying that has_body?/1 at

phoenix_live_reload/lib/phoenix_live_reload/live_reloader.ex

Line 157 in e8a8d80

if has_body?(resp_body) and :code.is_loaded(endpoint) do
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
Copy link

kuon commented Jan 8, 2024 •
edited

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
Copy link
Member

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
Copy link

kuon commented Jan 9, 2024

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
Copy link
Member

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@josevalim @kuon @chrismccord @wodow