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 WS sendImmediately #1949

Merged
merged 3 commits into from
Nov 16, 2023
Merged

Fix WS sendImmediately #1949

merged 3 commits into from
Nov 16, 2023

Conversation

matiboy
Copy link
Contributor

@matiboy matiboy commented Nov 3, 2023

Description

Fixes #1930.

When no sending element is provided to sendImmediately, the message is not sent to the socket, which contradicts the documentation which states that sendingElt is optional.

This PR:

  • Fixes the issue
  • Adds tests for correct current behavior of calling beforeSend and afterSend on the correct element if provided
  • Adds a test that would previously have failed (doesn't send if no sendElt provided)

To the reviewers, I've noticed that no second level "describe" is used to group the tests. In order to better focus on my own tests, it was easier for me to have a sub section of tests under ws extension. I've left it as is but please let me know if that's an issue.

@alexpetros alexpetros changed the title Issue 1930 Fix WS sendImmediately Nov 3, 2023
@alexpetros alexpetros linked an issue Nov 3, 2023 that may be closed by this pull request
@alexpetros
Copy link
Collaborator

Looks good, can you explain in more detail on the description what's bugged about the current behavior?

@matiboy
Copy link
Contributor Author

matiboy commented Nov 5, 2023

Looks good, can you explain in more detail on the description what's bugged about the current behavior?

Issue #1930 : when no sending element is provided to sendImmediately, the message is not sent to the socket, which contradicts the documentation which states that sendingElt is optional.

@@ -749,6 +749,24 @@ describe("web-sockets extension", function () {
window.document.removeEventListener("htmx:wsOpen", handler)
}
})
it('sends message if no sending element is provided', function (done) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@alexpetros This test is the one which would currently fail; no sending element would result in the message being gobbled up.

@alexpetros
Copy link
Collaborator

Added your comment to the description, hope that's alright. Just making it easier for 1cg to review.

@alexpetros alexpetros added bug Something isn't working ready for review Issues that are ready to be considered for merging labels Nov 5, 2023
@1cg
Copy link
Contributor

1cg commented Nov 16, 2023

@Renerick can you review, looks like a great change but I want you to see it as well before I merge.

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.

That was an unfortunate oversight on my part, haha. The fix looks good and tests are excellent. LGTM

@1cg 1cg merged commit c454ea4 into bigskysoftware:dev Nov 16, 2023
1 check passed
@matiboy matiboy deleted the issue-1930 branch November 18, 2023 08:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working ready for review Issues that are ready to be considered for merging
Projects
None yet
Development

Successfully merging this pull request may close these issues.

sendImmediately not sending data to the server
4 participants