-
-
Notifications
You must be signed in to change notification settings - Fork 806
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
Fixes to proxying; (a) blank pages when <body> not present, and (b) stalled connections with HTMX+hx-boost #604
Conversation
…by proxyHandler resulting in blank pages when no injection happened. Fix: change script injecting to inside <head> so as to not break HTMX' hx-boost body replacement due to exhausting available concurrent connection slots with reload-watching connections
if body == -1 { | ||
return page, false | ||
// the script will be injected before the end of the head tag. In case the tag is missing, the injection will be skipped. | ||
injectIdx := strings.LastIndex(page, "</head>") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if we didn't get body tag, then we try to find head tag
Can you explain why you have a blank page when the injection doesn't happen? If it doesn't find the body tag, it should proxy the unmodified response body. What issue are you solving here exactly? |
Can you move hx-boost to a div instead of directly in the body tag? |
Yeah sure; in the original code, the
The bug is that, if the content type was text/html, then My change fixes this by changing the meaning of the Hope this clarifies |
Yes, you could - hx-boost can be told to target any element iirc, using Simpler to for the air proxy just target a different location in the markup to inject to and then nobody needs to worry about it. (unless ofc there's some interaction or issue with other front-ends that I'm not aware of ofc - in which case happy to look for other solutions). Interestingly, browsersync.io's method injects near the beginning of but does not suffer the same bug - perhaps their JS method avoids leaving leftover connections alive, so maybe a fix could be made there instead. I haven't looked at the JS-side of things. |
Ok got it, I have a different idea on fixing this: #611 I think we're better off trying to do in a different order than, checking first when we can forward the original request. I am not convinced about moving the script to head? Javascript is traditionally injected before the end of body, that's the browser preference and it will do optimizations when reloading the page. I think air is more concerned to be aligned with the browser behaviour not just one library, and specially when there are ways you can make it work in your case. So I'd suggest discusing that separately in a dedicated issue/PR |
Fab, no worries, that's great :) (although just to clarify, the hx-boost thing is unique to HTMX - but not unique to my case; its the common-case use in HTMX, it doesn't matter which element the hx-boost attribute is placed on, the default behaviour is to replace in order to get SPA-like behaviour). |
I suppose a standard go templating strategy is to create a base template with html boilerplate (including head and body) and page templates. It wouldn’t be a good idea to place any htmx logic on the base template, each page will likely have their own logic and APIs to call. So I am still unconvinced in changing the script placement logic |
I am open for ideas, so thanks for bringing this up. For the time being we are better off concentrating on fixing this bug for now and discussing the script placement in a dedicated issue, also hearing other users of the proxy feature and we can make a better decision for the community |
Ha thanks guys, so glad to find this PR and the discussion is already going on, and a fix is on the way. |
This can be closed, my MR got merged. If you think we should move to head please create a separate issue |
fix: fixed
injectLiveReload()
modified
return incorrectly being used byproxyHandler()
resulting in blank pages when no injection happened. I.e.proxyHandler()
was testingmodified
and reading from the resp.Body reader if the content wasn't modified - exceptinjectLiveReload()
already read the body, so there's nothing else to read. This PR changes the semantics of themodified
return todidReadBuffer
, and fixes the problem; i.e. ifinjectLiveReload()
did read the body then use that, whether it was modified or not.Fix: change script injecting to inside so as to not break HTMX' hx-boost body replacement due to exhausting available concurrent connection slots with reload-watching connections. When the JS is added inside , then when using HTMX and hx-boost, hx-boost replaces in the same page for SPA-like behaviour - but the connection to the server for checking for reload remains open. After some navigating around the site, the browser's concurrent connection limit to the server is hit and the page becomes unresponsive. NB; this does change behaviour in that the location of the script injection is changed to be in - I don't think there are any side-effects to this and have not been able to reproduce any issue.