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

Fix SSE tests and emit new "beforeMessage" event for SSE Extension #2171

Merged
merged 5 commits into from
Jan 15, 2024

Conversation

neelrr1
Copy link
Contributor

@neelrr1 neelrr1 commented Jan 10, 2024

Description

I'm building a toy chat app with HTMX + the SSE extension. I have some custom JS that I want to run before and after new SSE messages are swapped in from the server.

For a regular request with HTMX, I could add listeners to the htmx:beforeSwap and htmx:afterSwap events. Similarly, the WS extension emits htmx:wsBeforeMessage and htmx:wsAfterMessage, but there is no corresponding ability to listen to "before swap" messages with the SSE extension.

This PR introduces a new event, htmx:sseBeforeMessage, that is emitted before the SSE extension swaps in content from the server. Open to feedback on naming convention!

Testing

I tested this change manually by applying this patch to my local copy of 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

@neelrr1 neelrr1 marked this pull request as ready for review January 10, 2024 09:20
@neelrr1 neelrr1 changed the title Emit htmx:sseBeforeMessage event for SSE Extension Emit new "beforeMessage" event for SSE Extension Jan 10, 2024
@neelrr1
Copy link
Contributor Author

neelrr1 commented Jan 10, 2024

It may also make sense to rename the sseMessage event to sseAfterMessage but that would be a breaking change.

@fhp-mec
Copy link
Contributor

fhp-mec commented Jan 12, 2024

Hi,

Given that the naming is at least consistent with the ws version it seems like the best choice for the name of the event if thats what is decided to be the best solution.

That said, I do think that the future of the sse extension would probably be to use htmx's built in swap function rather than the bespoke one that is currently being used, and therefore emit both before and after swap events consistent with the existing htmx behavior. (see #844 (comment) for context)

If having beforeSwap and afterSwap events be fired by the swapping function would give you the functionality you need I would suggest that instead of adding a new event.

I would also suggest that you add a regression test to the testing suite no matter what the solution is.

src/ext/sse.js Outdated Show resolved Hide resolved
@neelrr1 neelrr1 requested a review from Renerick January 15, 2024 07:13
@neelrr1
Copy link
Contributor Author

neelrr1 commented Jan 15, 2024

@Renerick, thanks so much for the first review and your comments. I've updated the code to add cancellation support and added tests to verify that the events are being emitted correctly. Can you take another look?

In the process, I also fixed our test suites for the SSE extension and hx-sse attribute. These were not running due to syntax errors in the test files, (#2167) which hid the fact that the tests were broken.

@neelrr1 neelrr1 changed the title Emit new "beforeMessage" event for SSE Extension Fix SSE tests and emit new "beforeMessage" event for SSE Extension Jan 15, 2024
Copy link
Collaborator

@Renerick Renerick left a comment

Choose a reason for hiding this comment

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

LGTM

Especially thank you for fixing syntax errors in SSE tests

Copy link
Collaborator

@Renerick Renerick left a comment

Choose a reason for hiding this comment

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

Github extension for VS code sent my approve twice, disregard this comment

@Renerick Renerick linked an issue Jan 15, 2024 that may be closed by this pull request
@neelrr1
Copy link
Contributor Author

neelrr1 commented Jan 15, 2024

@Renerick are you able to run CI so we can get this merged?

@1cg 1cg merged commit ac1d3f8 into bigskysoftware:dev Jan 15, 2024
1 check passed
Renerick added a commit to bigskysoftware/htmx-extensions that referenced this pull request Jan 15, 2024
Renerick added a commit to bigskysoftware/htmx-extensions that referenced this pull request Jan 15, 2024
Renerick added a commit to bigskysoftware/htmx-extensions that referenced this pull request Jan 15, 2024
Renerick added a commit to bigskysoftware/htmx-extensions that referenced this pull request Jan 15, 2024
Renerick added a commit to bigskysoftware/htmx-extensions that referenced this pull request Jan 15, 2024
1cg added a commit to bigskysoftware/htmx-extensions that referenced this pull request Jan 16, 2024
@neelrr1 neelrr1 deleted the sse-before-message-event branch January 16, 2024 07:14
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

Successfully merging this pull request may close these issues.

Missed syntax errors in test/ext/sse.js
4 participants