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

Fix Server Side Event Interrupted while the page was loading error message #2005

Closed
wants to merge 6 commits into from
Closed

Fix Server Side Event Interrupted while the page was loading error message #2005

Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
a890da0
Fix sse interrupted while the page was loading error
gungun974 Nov 18, 2023
ac39333
Don't use arrow function for ES5
gungun974 Nov 18, 2023
3b63882
Move beforeunload handler to `closeSSESourceBeforePageUnload` and rem…
gungun974 Nov 19, 2023
d4eac82
Revert "Move beforeunload handler to `closeSSESourceBeforePageUnload`…
gungun974 Nov 19, 2023
7c07f55
Replace spaces with tabs
gungun974 Nov 19, 2023
a0680a1
Dont refetch sseEventSource and use created source directly
gungun974 Nov 19, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions src/ext/sse.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,6 +135,14 @@ This extension adds support for Server Sent Events to htmx. See /www/extensions
var source = htmx.createEventSource(sseURL);
internalData.sseEventSource = source;

// Don't forget to disconnect the EventSource on page unload
window.addEventListener("beforeunload", function () {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we need to remove this event listener when SSE source is closed early, e.g. when its element is removed. I'd say, make the handler a named function and add invocation of removeEventListener("beforeunload", ... in maybeCloseSSESource

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I made the commit 3b63882 like you propose before reverting it when I realize a problem. (By the way this commit can't work)

I can't create a named function for the listener since I need to call api.getInternalData(elt) with the element.

I need to create if I want to externalize to an other function an another closure that take elt and pass to my named function. That don't solve that.

After I don't know how heavy is for the browser to keep eventListeners active but in my handler. I check if the EventSource is closed or not so doing what I do like this don't create problem on disconnecting a disconnected EventSource but I undertand I prefer also myself to clean up.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I see the problem, good catch. As for how to solve it, that may require a bit of a refactor, kinda like you describe. I can't figure out easy solution right now sadly, but hope to come up with something soon, or maybe someone else from the team has an idea in the meantime.

While the event listeners themselves are probably very lightweight, this particular handler creates a closure that holds elt variable, which most likely will prevent the browser from properly garbage collecting upon element removal. I haven't checked it though, so don't quote me on that.

Also, I noticed that you seem to have used spaces for indentation, but this file uses tabs, so please make sure that your fix is also indented with tabs.

Copy link
Author

@gungun974 gungun974 Nov 19, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for noticed this file was using tabs. To be honest I don't have any problem to use tabs VS space, it's just I never tough about that and so my neovim was configured with space for indent.

Anyway since I place my closure "beforeunload" in the same function that create the eventSource, maybe I don't need to refetch the InternalData and I can just reuse the same source. I don't know how exactly GC work in JS but I think using the variable source up scope directly should have the same behavior and except we don't need to track an element in GC. So I do what I said here in a0680a1.

When I said "it should" this is because I don't know yet how to write a Test that check our EventSource close method is called when we close the browser. But this is an issue of another conversation I think.

var source = api.getInternalData(elt).sseEventSource;
if (source != undefined) {
source.close();
}
});

// Create event handlers
source.onerror = function (err) {

Expand Down