-
Notifications
You must be signed in to change notification settings - Fork 70
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
[wasm][js] Dispose all listened events alongside with the application being disposed #1239
Conversation
import org.w3c.dom.events.Event | ||
import org.w3c.dom.events.EventTarget | ||
|
||
internal fun interface DisposableEventListener { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From https://developer.mozilla.org/en-US/docs/Web/API/EventTarget/removeEventListener:
Event listeners can also be removed by passing an AbortSignal to an addEventListener() and then later calling abort() on the controller owning the signal.
It seems like this is what you're trying to re-implement
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nope, this is intentional, AbortSignal has slightly different semantics and used basically for cancelling asynchronous operations (though on of consequences of it is indeed removing the event), removeEventListener is the standard way of removing events when they are no more needed. The other reason is that actually we have slightly more complicated scenarios that yet to be covered - like media query events which just won't work with AbortSignal
fun dispose() | ||
} | ||
|
||
internal fun EventTarget.addDisposableEvent(eventName: String, handler: (Event) -> Unit): DisposableEventListener { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this and DisposableEventListener
be private
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I actually agree, lesser is better, in theory one can use this separately but in practice it's almost always a bunch of events (and even in case of one event we always can use EventTargetListener)
pushed this changes
@@ -85,13 +93,17 @@ private interface ComposeWindowState { | |||
} | |||
} | |||
|
|||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Extra line
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
No description provided.