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.

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

Removing DOM element using SSE does not close the SSE connection #2510

Open
stevenengler opened this issue Apr 26, 2024 · 7 comments
Open

Removing DOM element using SSE does not close the SSE connection #2510

stevenengler opened this issue Apr 26, 2024 · 7 comments

Comments

@stevenengler
Copy link

stevenengler commented Apr 26, 2024

In the following example, I'm trying to delete #foo so that the SSE connection will close.

<div id="panel">
  <div id="foo" hx-ext="sse" sse-connect="/sse/foo" sse-swap="list"></div>
</div>

If I delete the div in developer tools (Chromium 121.0.6167.184) or using document.getElementById("panel").innerHTML='', the SSE connection stays open and never seems to close.

I'm using htmx 1.9.12.


What I'm actually trying to do is replace #foo with another SSE connection. But since browsers are limited to ~6 open connections at a time, I'm trying to close the existing connection before replacing it, so something like this:

<input type="radio" hx-trigger="change" hx-get="/bar" hx-target="#panel"
       hx-on::before-request="event.detail.target.innerHTML='';">

In this case the contents of #panel are cleared, but the browser developer tools show that the SSE connection is still open. This sometimes prevents the GET request to /bar if there are too many existing open connections, since the SSE connection hasn't closed.

@nickchomey
Copy link
Contributor

nickchomey commented Apr 26, 2024

Could you please explain your reasons for wanting to close the sse connection and open another? It would help us to help you achieve your goal.

Also, 6 connections is only for http1.1. If you use http 2 or 3, which are supported by essentially all people's browsers, then this is a non-issue.

Anyway, you should be able to use the maybeCloseEventSource function in your own function or event listener. Or, I'm pretty sure there's a way within htmx to trigger a function upon events.

function maybeCloseSSESource(elt) {

@stevenengler
Copy link
Author

stevenengler commented Apr 26, 2024

Could you please explain your reasons for wanting to close the sse connection and open another? It would help us to help you achieve your goal.

The page is designed so that as the user selects different inputs, the SSE div needs to be updated to use a different query parameter. There's too much incoming data to filter it only by the SSE message, so re-connecting with different parameters is used instead to filter and reduce the number of messages.

Also, 6 connections is only for http1.1. If you use http 2 or 3, which are supported by essentially all people's browsers, then this is a non-issue.

This is an application that can be run over localhost, so unfortunately only http/1.1 is used. Afaik you could technically use http/2 over localhost, but I don't think asking users to install a TLS cert for localhost is reasonable. This isn't a fault of htmx, but I'm just trying to mitigate the pain points as much as possible by trying to keep the number of SSE connections low. Initially I thought removing the existing element would be an easy way to do this.

Aside from that, it may be unintuitive to developers if deleting the dom element causes the connection to stay open. Does this mean the connection stays open until the page refreshes? If this is the intended behaviour, maybe it would be helpful to document.

Anyway, you should be able to use the maybeCloseEventSource function in your own function or event listener. Or, I'm pretty sure there's a way within htmx to trigger a function upon events.

Thanks, I'll look into that!

@stevenengler
Copy link
Author

Could you please explain your reasons for wanting to close the sse connection and open another? It would help us to help you achieve your goal.

I have another (probably better) reason now for wanting to close the sse connection. I have a panel (a div) which shows details of an item that's selected by a radio input. When the user selects a different radio input, the panel's contents are swapped out with a different sse connection to provide details for the selected item. This works fine, but the panel is also used for other things, where the panel's contents are overwritten with non-htmx/sse-related content. As the user selects the radio inputs and performs other actions which replace the panel's contents, the browser builds up a large number of unnecessary sse connections and leads to a lot of unnecessary data usage for the user.

@nickchomey
Copy link
Contributor

nickchomey commented Apr 30, 2024

It might be worth considering whether your backend architecture is appropriate. It seems to me that you should be able to have 1 sse connection and then the backend just sends whatever is necessary through it, depending on what ajax requests it has received.

@stevenengler
Copy link
Author

stevenengler commented Apr 30, 2024

It might be worth considering whether your backend architecture is appropriate. It seems to me that you should be able to have 1 sse connection and then the backend just sends whatever is necessary through it, depending on what ajax requests it has received.

There's too much data to send through a single sse connection for all items. It would use too much of the user's bandwidth, especially when server frameworks like axum+tower don't support compressed sse streams. It might make sense to use just javascript instead of htmx for this use case, or just poll instead of using sse.


I see that the sse extension seems to listen for htmx:beforeCleanupElement events:

htmx/dist/ext/sse.js

Lines 31 to 50 in b176784

/**
* onEvent handles all events passed to this extension.
*
* @param {string} name
* @param {Event} evt
* @returns void
*/
onEvent: function(name, evt) {
var parent = evt.target || evt.detail.elt;
switch (name) {
case "htmx:beforeCleanupElement":
var internalData = api.getInternalData(parent)
// Try to remove remove an EventSource when elements are removed
if (internalData.sseEventSource) {
internalData.sseEventSource.close();
}
return;

The documentation says:

Event - htmx:beforeCleanupElement

This event is triggered before htmx disables an element or removes it from the DOM.

It sounds to me like htmx.remove(document.querySelector("#foo")) (where #foo is the tag with hx-ext="sse") should trigger this event for the sse extension since htmx is removing it from the dom, which should cause the sse extension to close the event source. But the chromium debugger shows that this event isn't firing after calling the htmx.remove(...) (a tracepoint on line 44 isn't triggering). Is this a bug, or am I misunderstanding how this event works?

@stevenengler
Copy link
Author

The following patch properly cleans up a node when htmx.remove(elt) is called, which results in emitting a htmx:beforeCleanupElement event. This causes the sse connection to be properly closed.

diff --git a/dist/htmx.js b/dist/htmx.js
index 597c9b3b..55244a01 100644
--- a/dist/htmx.js
+++ b/dist/htmx.js
@@ -549,6 +549,7 @@ return (function () {
                     elt = null;
                 }, delay);
             } else {
+                cleanUpElement(elt);
                 elt.parentElement.removeChild(elt);
             }
         }

Here's the original function since the diff leaves out a lot of context:

htmx/dist/htmx.js

Lines 544 to 554 in b176784

function removeElement(elt, delay) {
elt = resolveTarget(elt);
if (delay) {
setTimeout(function(){
removeElement(elt);
elt = null;
}, delay);
} else {
elt.parentElement.removeChild(elt);
}
}


Would the project be open to a PR with this change? Is there a reason it wasn't done this way originally?

@nickchomey
Copy link
Contributor

Might as well submit that as a PR and see what they say!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants