websockets: added removeEventListener, modified existing related code and added a test - #6237
websockets: added removeEventListener, modified existing related code and added a test#6237Saad259 wants to merge 1 commit into
Conversation
… and added a test
Signed commits report1 of 1 commit between
This repository requires all commits to be signed. See GitHub docs on commit signature verification. |
|
|
| assert.NoError(t, err) | ||
| } | ||
|
|
||
| func TestRemoveEventListener(t *testing.T) { |
There was a problem hiding this comment.
Could you add cases for removeEventListener with a non-function/null handler, and for removing a listener that was registered twice via addEventListener? Both are silent no-ops right now and neither is covered here.
| } | ||
|
|
||
| func (w *webSocket) addEventListener(event string, handler func(sobek.Value) (sobek.Value, error)) { | ||
| func (w *webSocket) addEventListener(event string, handler sobek.Value) { |
There was a problem hiding this comment.
Suggestion:
The spec actually says to dedup here:
If eventTarget’s event listener list does not contain an event listener whose type is listener’s type, callback is listener’s callback, and capture is listener’s capture, then append listener to eventTarget’s event listener list.
Now that you introduced comparability, could you also add the dedup? This would also get rid of this somewhat strange behavior that one remove() call could wipe multiple listeners at once.
@ankur22 wdyt? IMO we could do this in this PR but on paper it would be a breaking change and somewhat out of scope.
There was a problem hiding this comment.
@janHildebrandt98 True true. I'll definitely add those test cases and I'd be happy to also work on dedup here itself (or under a new issue for it) if @ankur22 is happy with that.
2 participants
What?
This PR adds a
removeEventListenerto thek6/websocketsmodule. To support this,addEventListener's handler parameter was changed from an auto-converted Go closure (func(sobek.Value) (sobek.Value, error)) to a rawsobek.Value. This preserves the original JS function reference, which is needed to identify and remove a specific listener later (because a plain Go closure has no way to be compared back to the JS function it was created from). Each registered listener is now stored as asobek.Value/callable pair (listenerEntry), andremoveEventListenermatches on the incoming handler usingsobek.Value.SameAs().Why?
k6/websocketsis missing the counterpart toaddEventListener. This is useful in scenarios with many active listeners, where removing listeners that are no longer needed matters.Checklist
make check) and all pass.Checklist: Documentation (only for k6 maintainers and if relevant)
Please do not merge this PR until the following items are filled out.
Related PR(s)/Issue(s)
Closes #5877
Related: #4823 #4844