bigskysoftware / htmx-extensions

159 stars 46 forks source link

Add trigger 'htmx:sseClose' to sse close event #31

Closed sondalex closed 2 months ago

sondalex commented 3 months ago

This PR adds a new trigger, "htmx:sseClose", which is emitted when an SSE closing message is received.

netlify[bot] commented 3 months ago

Deploy Preview for htmx-extensions canceled.

Name Link
Latest commit 2e4cb173bf6cbb0559ec941df2ccb001d1f8a406
Latest deploy log https://app.netlify.com/sites/htmx-extensions/deploys/669034daa46ca3000875236e
Telroshan commented 3 months ago

Hey, while I like the idea, it seems you aren't firing this event in all cases where a source might be closed, was it intended? For ex I can see those places in the code where you didn't apply it: https://github.com/bigskysoftware/htmx-extensions/blob/8845604994bdae5b5dec0d5510732fd89f0a700c/src/sse/sse.js#L47-L49

https://github.com/bigskysoftware/htmx-extensions/blob/8845604994bdae5b5dec0d5510732fd89f0a700c/src/sse/sse.js#L239-L243

Also, do you think that, on error, a sseClose event should also be fired? (since it can close the event source, cf the reconnection algorithm)

Also, to accept this PR it'd be good to add the following :

sondalex commented 3 months ago

Hi, thank you for the review and recommendations! I understand your concerns about not firing the htmx:sseClose event in all cases where a source might be closed.

Initially, I intended to trigger the event only when a closing message is received, not when the connection is closed due to the removal of the element containing sse-connect. However, I see the value in firing the event in all closing situations.

Regarding your suggestion to fire the same event for all closing connections, I'm open to implementing htmx:sseClose for all cases. Alternatively, I could introduce separate events, such as htmx:sseNodeClose for node replacement and htmx:sseMessageClose for closing messages. What are your thoughts on this ?

Telroshan commented 3 months ago

Oh I see, makes sense! I think a parameter could make more sense? An enum-like value in the event's details that lets the user know, if it matters to them, what was the reason of this sse close. So that if someone just wants to bind some logic upon a sse connection closing, they don't have to bind 3 different events, and if they need to differentiate the scenarios, they may simply use if/else or a switch statement

sondalex commented 2 months ago

Hi @Telroshan , I have added test cases and implemented the close types in the event details. Thank you for the recommendation.

Users can access the sseClose details as follow:

document.body.addEventListener('htmx:sseClose', function (e) {
    const reason = e.detail.type
    switch(reason) {
        case "nodeMissing":
            // Parent node is missing and therefore connection was closed
            ...
        case "nodeReplaced":
            // Parent node replacement caused closing of connection 
            ...
        case "message":
            // connection was closed due to reception of message sse-close 
            ...
    }
})
Telroshan commented 2 months ago

Looks good @sondalex, thanks! Last request before we can merge this ; could you please remove the formatting changes? To concentrate the diff on the actual code changes

sondalex commented 2 months ago

Sure @Telroshan , I have just fixed it.

Telroshan commented 2 months ago

Perfect, thanks!