Fix: Make the SSE extension work properly and adhere to tests #2025

fhp-mec · 2023-11-22T00:04:48Z

Description

Add: tests for the sse extension
Partially Fix: sse:* inside of hx-trigger ⁽¹⁾
Fix: sse-swap events inside of previously swapped content

Previously sse-swap were only processed when sse-connect was on the root of what was being swapped in.
This is now fixed so that the closest sseEventSource is found and used as the event source.

While fixing this issue I ported the existing sse tests (attributes/hx-sse.js) to the sse extension, and while running those realized that the sse extension was not compliant with those tests, the change was one line so I decided to add it and make this a general sse extension fix pull request.

In some ways this is similar to the #1764 which seems to be stalled. However, it fixes a few more bugs, adds the missing testing.

Corresponding issues: #2023 #916

(1) I'm calling this a partial fix because actually supporting the full hx-trigger functionality for sse might not be possible without a substantial refactor of the trigger functionality in htmx itself. Right now the most basic usage of hx-trigger with sse events is supported (i.e. hx-trigger="sse:<event name>") however, comma separation of events, etc are not supported and can't really be supported unless there htmx allows new keywords and behavior to be added to hx-trigger by extensions. Or I copy the entirety of the behavior of hx-trigger into the sse extension which seems deeply silly and a real pain for the future maintainability of the sse extension.

Testing

I initially used the code in this gist to test this manually as there are no tests for the sse backend that I could find

I then realized that there were test cases in attributes/hx-sse.js so I copied those, edited them to work with the sse extension.

Checklist

I have read the contribution guidelines
I have targeted this PR against the correct branch (master for website changes, dev for
source changes)
This is either a bugfix, a documentation update, or a new feature that has been explicitly
approved via an issue
I ran the test suite locally (npm run test) and verified that it succeeded

Previously `sse-swap` were only processed when `sse-connect` was on the root of what was being swapped in. This is now fixed so that the closest sseEventSource is found and used as the event source

remove the nested if statements as there is no difference in handling between sseURL and LegacySSEUrl.

+ more tests differentiating the implementation of sse-swap and hx-trigger * fix for "is closed after removal with no close and activity"

fhp-mec · 2023-11-24T23:16:38Z

This should be ready to go now. In a rush I commited the test but not the fix for the test.

Test coverage is decent at this point

Renerick · 2023-12-02T09:26:49Z

Hi! I was looking into related SSE PR and it appears that you got potentially proper fix for the important issue of sse-swap initialization, so I will be doing the review, as I think the issue is basically critical and should be fixed ASAP. At a quick glance, I noticed that your changes are indented with spaces instead of tabs, please fix that.

Since it's a medium-to-large PR, I'll be properly focusing on the review during the following week. In the meantime, I provided some comments about current state of the extension and ideas for fixes in this comment #1764 (comment). Please see if there is something useful here and consider incorporating those proposals in your PR.

Thank you for your contribution!

This reverts commit bbe3715.

fhp-mec · 2023-12-02T11:05:55Z

I was looking into related SSE PR and it appears that you got potentially proper fix for the important issue of sse-swap initialization, so I will be doing the review, as I think the issue is basically critical and should be fixed ASAP. At a quick glance, I noticed that your changes are indented with spaces instead of tabs, please fix that.

Just fixed that, thanks for bringing it to my attention.

Since it's a medium-to-large PR, I'll be properly focusing on the review during the following week. In the meantime, I provided some comments about current state of the extension and ideas for fixes in this comment #1764 (comment). Please see if there is something useful here and consider incorporating those proposals in your PR.

This made for some interesting reading. I think I came to a similar conclusion that you did with respect to #916/#1764. However, instead of creating a new function I reorganized the if statement to populate it with the closest event source in the dom.

I implemented the fix in this way instead of removing the sse-swap and hx-trigger functionality to a wholly separate function to try to limit the amount of code I was touching.
Unfortunately I quickly found out that there was no testing for the sse extension, so I added testing up to parity with the legacy implementation, and found that the extension did not replicate the functionality, so the scope of this pull request ballooned and here we are.
I'm not opposed to changing it to have separate function(s) to fill out the sse-swap and hx-trigger functionality (the way hx-trigger is implemented now is a kludge anyway as we really need an api for adding custom behavior to hx-trigger to do it properly and have it play nice with other extensions and the default implementation, htmx2.0 perhaps).

I look forward to your review.

Renerick

First of all, I'm sorry I took this long to leave my response and thank you for taking the time to work on improving SSE extension. Adding tests for the extension is HUGE improvement and will definitely help us with maintaining the extension.

As for the fixes, I many your changes definitely positive, but unfortunately, I also found some cases when the extension still behaves not the way users would expect, which I detailed in the code comments. About, separating code into separate functions vs. keeping changes minimal, I very much respect your approach and will not insist on doing it exactly this way, but I also won't object to it, if you find such changes necessary. But i'm still convinced that having sse-connect search performed on the entire tree of swapped content is crucial for the extension and this is the first thing that should be fixed.

Best regards, and thank you once again for contributing to htmx!

src/ext/sse.js

test/ext/sse.js

+ seperate eventSource creation logic and event registering logic + manually create event handling, still confused by how hx-trigger works * `createEventSourceOnElement` now looks for event sources in children * explicitly handle legacy sse handling instead of having extra selectors in `querySelectorOnThisOrChildren` + a few readability changes + add regression check to make sure that sseEventSource is only created on elements with sse-connect or equivalent + add test to make sure that sse-connect in the child of a swapped element is handled

fhp-mec · 2023-12-09T20:12:17Z

As for the fixes, I many your changes definitely positive, but unfortunately, I also found some cases when the extension still behaves not the way users would expect, which I detailed in the code comments.

I think I've resolved almost all of your comments that I didn't have questions about in my latest commit.

About, separating code into separate functions vs. keeping changes minimal, I very much respect your approach and will not insist on doing it exactly this way, but I also won't object to it, if you find such changes necessary.

I've now separated the eventSource creation logic from the event registering logic. On reflection now that the scope has spiraled wildly, its better to do the right thing then to try to limit scope creep :)

Best regards, and thank you once again for contributing to htmx!

Thanks for your diligence and your work on this fantastic library!

I look forward to your further comments.

Renerick

Ok, I've reviewed the latest changes, and all seems to be in order. My only nitpick would be to rename registerSSE into something more explicit, like registerSSEEventHandlers for example. There is also a lot of some whitespace changes which aren't strictly necessary, but I'm fine with it. Would be great to also get some feedback from @benpate, as for me, I approve this PR

1cg · 2023-12-14T20:46:43Z

@fhp-mec can you confirm that this change is backwards compatible? (I don't know this code well enough to be sure)

1cg · 2023-12-14T21:35:14Z

merged per @Renerick's endorsement, @fhp-mec would still like your comments on backwared compat.

thank you for your work on this!

fhp-mec · 2023-12-14T23:42:42Z

@1cg

I'm relatively confident in saying its backwards compatible with 1.9.6, I dropped it into a thing that I had deployed a little while ago and it just worked.
That said it only used the swapping functionality, though I think it may have pretty much covered everything that was working in 1.9.6.

I'm not super familiar with the javascript support on ie11 so I tried to copy the syntax used in the rest of htmx and use as many of the existing api endpoints as possible. I'm not however setup to test on ie11, so that's the only danger point I think.

rbri · 2024-01-08T18:47:44Z

test/attributes/hx-sse.js

+                        var event = htmx._("makeEvent")(eventName);
+                        event.data = data;
+                        listener(event);
+                    }


closing ')' missing -> syntax error

rbri · 2024-01-08T18:48:23Z

test/ext/sse.js

+                        var event = htmx._("makeEvent")(eventName);
+                        event.data = data;
+                        listener(event);
+                    }


and again closing ')' missing

fhp-mec added 2 commits November 21, 2023 15:47

Process sse-swap properly when swapped in

271e022

Previously `sse-swap` were only processed when `sse-connect` was on the root of what was being swapped in. This is now fixed so that the closest sseEventSource is found and used as the event source

Pass for code readabiltiy

c4290a8

remove the nested if statements as there is no difference in handling between sseURL and LegacySSEUrl.

fhp-mec changed the title ~~Process sse-swap properly when swapped in~~ Fix: Process sse-swap properly when swapped in Nov 23, 2023

fhp-mec added 4 commits November 22, 2023 17:56

Fix indenting

b200937

Initial attempt at sse extension tests

97dfb72

Make existing tests pass

cf2081e

Add test case for bigskysoftware#916

be8b209

fhp-mec changed the title ~~Fix: Process sse-swap properly when swapped in~~ Fix: Make the SSE extension work properly and adhere to tests Nov 23, 2023

fhp-mec added 3 commits November 23, 2023 04:45

backport test for bigskysoftware#916 to hx-sse for completeness

4a05e39

add a distinct test for sse-swap

1709915

more tests, fix tests

9954dbe

+ more tests differentiating the implementation of sse-swap and hx-trigger * fix for "is closed after removal with no close and activity"

fhp-mec and others added 2 commits November 26, 2023 01:13

Merge branch 'bigskysoftware:dev' into dev

d1c88e6

Cleanup listener if element is gone

bbe3715

Renerick mentioned this pull request Dec 2, 2023

fix(sse): actually re-use event source and register swapped children #1764

Closed

fhp-mec added 2 commits December 2, 2023 01:39

Revert "Cleanup listener if element is gone"

2301639

This reverts commit bbe3715.

Clean up listeners if element gone, fix indenting

141d534

Merge branch 'bigskysoftware:dev' into dev

22baa8f

alexpetros assigned Renerick Dec 8, 2023

Renerick reviewed Dec 9, 2023

View reviewed changes

src/ext/sse.js Outdated Show resolved Hide resolved

src/ext/sse.js Outdated Show resolved Hide resolved

src/ext/sse.js Show resolved Hide resolved

Renerick reviewed Dec 9, 2023

View reviewed changes

test/ext/sse.js Outdated Show resolved Hide resolved

fhp-mec and others added 3 commits December 9, 2023 13:36

Allow multiple listeners in eventsource mock

eb058e6

Backport eventsource mocks to legacy tests

7e42105

Merge branch 'bigskysoftware:dev' into dev

06c88d5

Renerick approved these changes Dec 14, 2023

View reviewed changes

Renerick added the ready for review Issues that are ready to be considered for merging label Dec 14, 2023

Renerick added enhancement New feature or request extension Consideration for an extension labels Dec 14, 2023

Renerick mentioned this pull request Dec 14, 2023

Remove legacy WS/SSE code #2095

Merged

4 tasks

Merge branch 'dev' into dev

584a776

1cg merged commit 11cef8e into bigskysoftware:dev Dec 14, 2023
1 check passed

Renerick mentioned this pull request Dec 14, 2023

fix conflicting function names in sse.js #2096

Merged

4 tasks

rbri reviewed Jan 8, 2024

View reviewed changes

test/ext/sse.js

var event = htmx._("makeEvent")(eventName);

event.data = data;

listener(event);

}

Copy link

Contributor

rbri Jan 8, 2024

Choose a reason for hiding this comment

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

and again closing ')' missing

Renerick mentioned this pull request Jan 9, 2024

Missed syntax errors in test/ext/sse.js #2167

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix: Make the SSE extension work properly and adhere to tests #2025

Fix: Make the SSE extension work properly and adhere to tests #2025

fhp-mec commented Nov 22, 2023 •

edited

Loading

fhp-mec commented Nov 24, 2023

Renerick commented Dec 2, 2023

fhp-mec commented Dec 2, 2023 •

edited

Loading

Renerick left a comment

fhp-mec commented Dec 9, 2023

Renerick left a comment

1cg commented Dec 14, 2023

1cg commented Dec 14, 2023

fhp-mec commented Dec 14, 2023

rbri Jan 8, 2024 •

edited

Loading

rbri Jan 8, 2024

Fix: Make the SSE extension work properly and adhere to tests #2025

Fix: Make the SSE extension work properly and adhere to tests #2025

Conversation

fhp-mec commented Nov 22, 2023 • edited Loading

Description

Testing

Checklist

fhp-mec commented Nov 24, 2023

Renerick commented Dec 2, 2023

fhp-mec commented Dec 2, 2023 • edited Loading

Renerick left a comment

Choose a reason for hiding this comment

fhp-mec commented Dec 9, 2023

Renerick left a comment

Choose a reason for hiding this comment

1cg commented Dec 14, 2023

1cg commented Dec 14, 2023

fhp-mec commented Dec 14, 2023

rbri Jan 8, 2024 • edited Loading

Choose a reason for hiding this comment

rbri Jan 8, 2024

Choose a reason for hiding this comment

fhp-mec commented Nov 22, 2023 •

edited

Loading

fhp-mec commented Dec 2, 2023 •

edited

Loading

rbri Jan 8, 2024 •

edited

Loading