RAII system for handling event listeners

[Pauan]

Summary

I propose to implement an API which makes it easy to use Rust's RAII-based system for event listener registration/deregistration.

Motivation

When registering an event listener, it is often needed to deregister it later.

In JavaScript this is done with the removeEventListener method, but that is verbose and error prone. It also does not clean up Rust resources (e.g. the memory for the closure).

Detailed Explanation

pub struct EventListener<'a, F> where F: ?Sized {
    node: EventTarget,
    kind: &'a str,
    callback: Closure<F>,
}

impl<'a, F> EventListener<'a, F> where F: ?Sized + WasmClosure {
    #[inline]
    pub fn new<F>(node: &EventTarget, kind: &'a str, f: F) -> Self {
        let callback = Closure::wrap(Box::new(f) as Box<F>);

        node.add_event_listener_with_callback(kind, callback.as_ref().unchecked_ref()).unwrap();

        Self {
            node: node.clone(),
            kind,
            callback,
        }
    }
}

impl<'a, F> Drop for EventListener<'a, F> where F: ?Sized {
    #[inline]
    fn drop(&mut self) {
        self.node.remove_event_listener_with_callback(self.kind, self.callback.as_ref().unchecked_ref()).unwrap();
    }
}

The idea is that you can simply call EventListener::new(&node, "foo", move |e| { ... }) and when the EventListener is dropped it will then cleanup the Rust memory for the closure and it will also deregister the event listener.

This is a general purpose idiomatic mid-level API which can work with any event on any DOM node. I expect it to be used pervasively both within Gloo and outside of Gloo.

Drawbacks, Rationale, and Alternatives

For a mid-level API like this, there aren't really any alternatives. Some small things can be tweaked, and I haven't thought very hard about how to support FnOnce (but I'm sure it is possible). Ideas and refinements are welcome.

Unresolved Questions

It is possible to build a higher level API on top of this.

This higher level API gives full static type support for JS events, making it impossible to get the types wrong.

The way that it works is that a new trait like this is created:

pub trait StaticEvent {
    const TAG: &'static str;
    type Event;
    fn wrap(event: Self::Event) -> Self;
}

And a new fn is created, like this:

pub fn on<A, F>(node: &EventTarget, f: F) -> EventListener<'static, FnMut(A::Event)>
    where A: StaticEvent,
          F: FnMut(A) + 'static {
    EventListener::new(node, A::TAG, move |e| {
        f(A::wrap(e));
    })
}

Now you can create various events like this:

struct ClickEvent {
    event: web_sys::MouseEvent,
}

impl ClickEvent {
    // Various methods specific to the `click` event
}

impl StaticEvent for ClickEvent {
    const TAG: &'static str = "click";

    type Event = web_sys::MouseEvent;

    fn wrap(event: Self::Event) -> Self {
        ClickEvent { event }
    }
}

struct MouseDownEvent {
    event: web_sys::MouseEvent,
}

impl MouseDownEvent {
    // Various methods specific to the `mousedown` event
}

impl StaticEvent for MouseDownEvent {
    const TAG: &'static str = "mousedown";

    type Event = web_sys::MouseEvent;

    fn wrap(event: Self::Event) -> Self {
        MouseDownEvent { event }
    }
}

And now finally we can actually use the API:

on(&node, move |e: ClickEvent| {
    ...
})

on(&node, move |e: MouseDownEvent| {
    ...
})

This API has some huge benefits:

• You no longer need to specify a string for the event type (so no more typos).

• You no longer need to do any coercions to the correct type (that's all handled automatically now).

• It guarantees that the static type is correct (in other words, if you have a click event, it is guaranteed to be matched to a ClickEvent Rust type, and vice versa).

This technique was pioneered by stdweb, and it works out really well in practice. It may also be necessary for solving rustwasm/wasm-bindgen#1348

But that's something which can be layered on top of the mid-level EventListener API, so I defer it to later.

[Pauan]

Oh, also, similarly to Closure, there should be a forget method on EventListener which permanently leaks the memory.

Of course this should be used carefully, but sometimes you truly do want an event listener to last for the duration of your entire program.

[fitzgen]

(Ignoring the potential higher-level API, and focusing on the actual proposal for now)

I like this and think we should move forward with it!

Nitpicks:

• Probably s/node/target/ since there are things that are not DOM nodes that are still event targets, notably window.
• I think we should have a forget method, as you say, that is similar to wasm_bindgen::Closure's.
• We should avoid using the WasmClosure trait in the public API -- that's a semi-unfortunate internal detail to wasm-bindgen.
• We should avoid taking ?Sized parameters directly, and not behind a reference. That requires the unsized nightly feature, and I think we should target stable rust, given that our 2019 roadmap calls out stability as a goal. We can always add support post-facto as the unsized feature become stable.
• Given those last two points, we should make EventListener::new take a function F: 'static + FnMut(&web_sys::Event)

Do you have thoughts, @David-OConnor @alexcrichton @yoshuawuyts @ashleygwilliams?

[alexcrichton]

This all sounds great to me! I'd echo @fitzgen's points about WasmClosure being a somewhat unfortunate detail which would be great if we could avoid. If another design can't be figure out though which doesn't have the same ergonomics we could probably make do with adding a trait bound of it.

[David-OConnor]

Love it. If I understand correctly, this would tackle several issues:

• Constraining events to valid ones
• Elegantly handle closure storage/dropping/memory-issues
• Casting generic web_sys::Events as specific ones, which is automatic in JS, but verbose in web_sys
• Wrapping web_sys's verbose listener add/remove API

Could use macros to populate the events.

[Pauan]

Probably s/node/target/ since there are things that are not DOM nodes that are still event targets, notably window.

Good catch.

We should avoid using the WasmClosure trait in the public API -- that's a semi-unfortunate internal detail to wasm-bindgen.

We should avoid taking ?Sized parameters directly, and not behind a reference. That requires the unsized nightly feature, and I think we should target stable rust, given that our 2019 roadmap calls out stability as a goal. We can always add support post-facto as the unsized feature become stable.

I agree, though I'm concerned about the ergonomics and flexibility of the API.

Given those last two points, we should make EventListener::new take a function F: 'static + FnMut(&web_sys::Event)

Shouldn't that be web_sys::Event (no reference)?

[Pauan]

@David-OConnor To be clear, this proposal is just for EventListener, the actual type-safe API is a potential undecided future extension (though one that I'm obviously in favor of).

As for a macro, yeah, that's exactly what stdweb does: it's literally 1 line of code to generate the StaticEvent impl.

[Pauan]

One thing I would like to note: if EventListener always gives a web_sys::Event to the closure, then that's a big ergonomics hit, since it means that users now need to manually cast the type, whereas with the raw add_event_listener API they don't.

So in that case my StaticEvent proposal becomes much more important for the sake of ergonomics.

Do we want to discuss that now, or wait until we get more experience with using EventListener first?

[Pauan]

I thought of something: rather than putting a WasmClosure trait bound, what if it used JsCast? That would allow for the API to automatically cast into any JsCast type (which includes all of the event types).

That still gives full flexibility, and avoids manual type casts, but it also avoids the WasmClosure bound.

[fitzgen]

I thought of something: rather than putting a WasmClosure trait bound, what if it used JsCast? That would allow for the API to automatically cast into any JsCast type (which includes all of the event types).

So something like

pub fn new<F, T>(target: &EventTarget, kind: &'a str, f: F) -> EventListener
where
    F: FnMut(T),
    T: JsCast,
{ ... }

?

Would this do dynamically-checked or unchecked casts? I guess, as written above, it would have to be unchecked.

I'm of two minds on this. On one hand, I'd also like if users weren't forced to use JsCast themselves. On the other, the above signature lets you put any old plainly wrong type there (like js_sys::WebAssembly::Module) and it will seemingly work...

I guess I am leaning towards using web_sys::Event because then the user is not "lied to" and they can choose the casting that makes sense for them.

Shouldn't that be web_sys::Event (no reference)?

It should be cheaper at the ABI boundary to take a reference rather than ownership, and one can always .clone() to get an owned handle.

[fitzgen]

I guess I am leaning towards using web_sys::Event because then the user is not "lied to" and they can choose the casting that makes sense for them.

And we can explore potential follow ups, like in your unresolved questions, that solve the casting problem in a better manner.

[Pauan]

So something like

Yes, exactly.

Would this do dynamically-checked or unchecked casts?

I was thinking dynamically-checked, with a separate unchecked_ method for unchecked casts.

I guess I am leaning towards using web_sys::Event because then the user is not "lied to" and they can choose the casting that makes sense for them.

If we don't do automatic casts then that's a big hit to ergonomics, and makes things more error prone.

Since event listening is so common, we need a good ergonomic solution. So if EventListener isn't going to be that solution, then in my opinion we should start seriously exploring StaticEvent (or similar).

If we have consensus that something like StaticEvent is desired, then I can make a new issue for that.

It should be cheaper at the ABI boundary to take a reference rather than ownership

Woah, I didn't realize closure argument references behaved like that. I thought that only applied to regular functions. In that case that's a big 👍 from me.

[yoshuawuyts]

Coming in late here, but I really like this direction. Very excited for this!

[fitzgen]

Since event listening is so common, we need a good ergonomic solution. So if EventListener isn't going to be that solution, then in my opinion we should start seriously exploring StaticEvent (or similar).

If we have consensus that something like StaticEvent is desired, then I can make a new issue for that.

👍

Unless anyone would like to voice concerns, I think we are ready to

• open a PR for the mid-level events layer proposed here, using FnMut(&web_sys::Event) Done; see [WIP] gloo-events #42
• open a new issue discussing/proposing the higher-level API with static type guarantees Done; see A high-level, convenient, and safe, statically typed event system #43

Did you want to tackle these @Pauan? Alternatively, we can write out some bullet points / skeleton and make it into a good first issue

[Pauan]

@fitzgen When I tried to implement this, I found out that Closure does not allow for references in its arguments.

@alexcrichton Is that intentional, or just an omission? If it's an omission, should I wait for reference support in wasm-bindgen before implementing this?

[Pauan]

WIP PR up: #42

Let's discuss some of the minor details there.

[Pauan]

High level design posted: #43

[alexcrichton]

@alexcrichton Is that intentional, or just an omission? If it's an omission, should I wait for reference support in wasm-bindgen before implementing this?

Oh oops sorry missed this!

This is currently just an omission because it requires more impl blocks. Should be fine to add at any time!

(note that it currently requires a combinatorial explosion of impl blocks because these are distinct IIRC):

impl<A, B> Trait for Fn(A, B)
impl<A, B> Trait for for<'a> Fn(&'a A, B)
impl<A, B> Trait for for<'b> Fn(A, &'b B)
impl<A, B> Trait for for<'a, 'b> Fn(&'a A, &'b B)

[Pauan]

@alexcrichton Does it? Can't it use a trait like this?

trait WasmRefArgument {
    // ...
}

impl<A> WasmRefArgument for A { ... }
impl<'a, A> WasmRefArgument for &'a A { ... }

And then...

impl<A, B> WasmClosure for Fn(A, B) where A: WasmRefArgument, B: WasmRefArgument { ... }

Or does that not work?

[alexcrichton]

Hm perhaps! I basically just remember spending way too long getting it to work in the beginning and endlessly running into walls. I'm not entirely proud of the current state of things (new vs wrap), so if there's ideas of how to improve it I'm all for them :)

I think whatever works for supporting references is fine to implement as well, it should all be backwards compatible too!

[fitzgen]

I think for the situation at hand, we can use owned versions of web_sys::Event in the callback before we publish the gloo_events crate, and just make sure we get this fixed upstream in wasm-bindgen before publishing (with a tracking issue here, so we don't forget).

How does that sound?

[Pauan]

@fitzgen Switching from Event to &Event in gloo-events is a breaking change. Are we okay with doing that?

EDIT: Oops, I reread what you said, and I agree. 👍