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

penguin proxy does not seem to insert the autoreload javascript #11

Closed
dreuter opened this issue Nov 16, 2023 · 5 comments
Closed

penguin proxy does not seem to insert the autoreload javascript #11

dreuter opened this issue Nov 16, 2023 · 5 comments

Comments

@dreuter
Copy link

dreuter commented Nov 16, 2023

Hey :)

First of all I have to say I really love your project. I used it now in a couple of my personal projects as a static file server and have to say it works fantastic for that. Thanks so much, you have already saved me dozens of development hours.

I only encountered one "problem" so far. When I use penguin proxy it seems to not insert the autoreload script into my html.
Is that on purpose? Do I do something wrong?

To double check I have saved the exact same html into a file and served that through penguin and then the javascript will be inserted, which leads me to believe that it has nothing to do with the actual html I send.

The workaround is simple, as I can just add the link to the javascript to my templates, but ideally I would like my service to not know about penguin (although that is also already not that easy because of the autoreload needing to be triggered "manually").
@LukasKalbertodt
Copy link
Owner

Hello! Thanks a lot for those kind words. It really means a lot hearing people find value in my projects :)

Regarding your problem: my bet is that your origin server does not properly set the Content-Type header.

penguin/lib/src/serve/proxy.rs

Lines 123 to 126 in 37fc03d

let is_html = content_type.map_or(false, |v| v.as_ref().starts_with(b"text/html"));
if !is_html {
return response;
}

The script is only injected if the content type is set to text/html. Or am I missing a content type that should also get the script injected? Additionally, it might be possible to look at the body if the content type is not set at all, to determine if it's likely HTML?

Is my guess correct?

@dreuter
Copy link
Author

dreuter commented Nov 16, 2023

Yes your guess is 100% correct. Thanks a lot!

I think what you could do additionally is check if the document starts with <!DOCTYPE html> and/or contains <meta http-equiv="Content-Type" content="text/html.*">, although the latter one would probably be quite slow (but would be closer to the browser behavior).

Now that I know that I should set that header for me it is more than fine to not change anything in penguin. If you want I could create a CR mentioning that this header is required in the readme.

LukasKalbertodt added a commit that referenced this issue Nov 26, 2023
@LukasKalbertodt
Add body sniffing in proxy-mode to detect HTML more often
3104171 
A common pitfall was that the origin server didn't set the Content-Type
correctly, making Penguin not insert the reload script into HTML. The
Content-Type header should be correctly set, but Penguin can improve
user experience here anyway, by sniffing and checking if the body looks
like HTML. Specifically: if the body looks like HTML, but there is no
Content-Type header, we treat it as HTML and warn. If it looks like HTML
but the Content-Type header says something else, we warn and treat it
like non-HTML.

Addresses #11
@LukasKalbertodt
Copy link
Owner

@dreuter I justed pushed some commits to main. This should be improved now. I did personally run into this problem as well, being confused as to why I'm not getting auto-reloading. So yes, improving the situation was a good idea.

If you want, you could test the new version with cargo install -f --git https://github.com/LukasKalbertodt/penguin. But I will likely release a new version soon or momentarily, depending on whether I will still update dependencies.

@dreuter
Copy link
Author

dreuter commented Nov 26, 2023

Since I now know to set the header, my application is doing it and penguin worked well (I think it is the right thing to do anyhow).

But just to confirm, I reverted the commit setting the header and et voila, penguin is printing this warning:

 WARN  penguin::serve::proxy > Proxy response to '/recipes/' looks like HTML, but no 'Content-Type' header exists. I will treat it as HTML (injecting reload script), but setting the correct 'Content-Type' header is recommended.

The script is still in the response and everything is working perfectly. Thanks so much!

I am closing the issue, but feel free to reopen it, if I have missed anything :)

@dreuter dreuter closed this as completed Nov 26, 2023
@LukasKalbertodt
Copy link
Owner

LukasKalbertodt commented Nov 26, 2023 •
edited
Loading

Neat, thanks for the quick test! Will release it later today then. (Edit: Released as 0.1.8 & 0.2.6)

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
2 participants
@dreuter @LukasKalbertodt