Introduce new AddEventListener and RemoveEventListener APIs on JSObject #55849
Conversation
|
I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label. |
|
Tagging subscribers to 'arch-wasm': @lewing Issue DetailsCode does event listener management kind of ad-hoc right now, and this creates a lot of space for lifetime management or removal to get messed up. This is a draft of new APIs to simplify things and make it harder to make mistakes.
|
...Runtime.InteropServices.JavaScript/src/System/Runtime/InteropServices/JavaScript/JSObject.cs
Outdated
Show resolved
Hide resolved
lewing
left a comment
lewing
left a comment
There was a problem hiding this comment.
Nullable annotations appear to be wrong.
|
I believe this is ready for review. The new design feels okay to me, and most importantly it's simple. I've been looping the websocket tests for about 10 hours now and they haven't failed yet. |
| return; | ||
| } | ||
| }, | ||
| fireEvent: function (name, evt) { |
There was a problem hiding this comment.
Is there a test using the evt?
There was a problem hiding this comment.
I could add one, but it's not really necessary since this API doesn't actually touch the event dispatch part
....Runtime.InteropServices.JavaScript/src/System/Runtime/InteropServices/JavaScript/Runtime.cs
Outdated
Show resolved
Hide resolved
....Runtime.InteropServices.JavaScript/src/System/Runtime/InteropServices/JavaScript/Runtime.cs
Outdated
Show resolved
Hide resolved
...nteropServices.JavaScript/tests/System/Runtime/InteropServices/JavaScript/JavaScriptTests.cs
Show resolved
Hide resolved
....Runtime.InteropServices.JavaScript/src/System/Runtime/InteropServices/JavaScript/Runtime.cs
Outdated
Show resolved
Hide resolved
| if (options?.Signal != null) | ||
| throw new NotImplementedException("EventListenerOptions.Signal"); | ||
|
|
||
| var jsfunc = Runtime.GetWeakDelegateHandle(listener); |
There was a problem hiding this comment.
If we could wrap the listener with our own lambda here and call ReleaseInFlight(arg1) we would stop leaking inflight JSObjects of the event argument (which I introduced recently).
There was a problem hiding this comment.
I will look into this, great idea
|
Please add test of registering same delegate to multiple event sources and it's unregistration. |
I did, it's called RegisterSameEventListener |
That's same delegate twice into same source. I'm asking for same delegate into multiple source instances. |
ghost
locked as resolved and limited conversation to collaborators
4 participants
Code does event listener management kind of ad-hoc right now, and this creates a lot of space for lifetime management or removal to get messed up. This is a draft of new APIs to simplify things and make it harder to make mistakes.