search

air-verse / air

☁️ Live reload for Go apps
GNU General Public License v3.0
16.32k stars 770 forks source link

Fixes to proxying; (a) blank pages when <body> not present, and (b) stalled connections with HTMX+hx-boost #604

Open chivaesi opened 4 weeks ago

chivaesi commented 4 weeks ago

fix: fixed injectLiveReload() modified return incorrectly being used by proxyHandler() resulting in blank pages when no injection happened. I.e. proxyHandler() was testing modified and reading from the resp.Body reader if the content wasn't modified - except injectLiveReload() already read the body, so there's nothing else to read. This PR changes the semantics of the modified return to didReadBuffer, and fixes the problem; i.e. if injectLiveReload() 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.

ndajr commented 3 weeks ago

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?

ndajr commented 3 weeks ago

Can you move hx-boost to a div instead of directly in the body tag?

chivaesi commented 3 weeks ago

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?

Yeah sure; in the original code, the if modified { block (line 137) sends page if it was modified, but if it was not modified then line 144 sends from the resp.Body reader:

if _, err := io.Copy(w, resp.Body); err != nil {

The bug is that, if the content type was text/html, then resp.Body was already read from (in order to look for ), so there is nothing left to read. Resulting in nothing/empty being written through to the response.

My change fixes this by changing the meaning of the modified return to instead signify whether resp.Body had been read from already. Arguably with this there could be further simplification of the logic/structure, but I didn't want to propose too big a patch to lose sight of the actual fix.

Hope this clarifies

chivaesi commented 3 weeks ago

Can you move hx-boost to a div instead of directly in the body tag?

Yes, you could - hx-boost can be told to target any element iirc, using hx-target. However it feels better not to have to ask all htmx developers to do something non-default (and add extra markup) to work around this - especially when the symptom/problem it causes is "weird"; i.e. stalled/hung connections, no error message, no visible connections in Chrome dev tools - i.e. its hard to a developer to ascertain that this is the reason.

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.

ndajr commented 3 weeks ago

Ok got it, I have a different idea on fixing this: https://github.com/air-verse/air/pull/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

chivaesi commented 3 weeks ago

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).

ndajr commented 3 weeks ago

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

ndajr commented 3 weeks ago

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

kefniark commented 3 weeks ago

Ha thanks guys, so glad to find this PR and the discussion is already going on, and a fix is on the way. It was driving me crazy for the past hours, why some of my routes were suddenly returning white response with 0 bytes only in development 😢 And my debugging session end up at the same place, air proxy injection code.

ndajr commented 4 days ago

This can be closed, my MR got merged. If you think we should move to head please create a separate issue