RAII system for handling event listeners about gloo HOT 21 CLOSED

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 #42
• open a new issue discussing/proposing the higher-level API with static type guarantees Done; see #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

from gloo.

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!

from gloo.

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.

from gloo.

(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?

from gloo.

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.

from gloo.

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.

from gloo.

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)?

from gloo.

@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.

from gloo.

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?

from gloo.

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.

from gloo.

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.

from gloo.

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.

from gloo.

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.

from gloo.

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

from gloo.

@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?

from gloo.

WIP PR up: #42

Let's discuss some of the minor details there.

from gloo.

High level design posted: #43

from gloo.

@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)

from gloo.

@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?

from gloo.

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?

from gloo.

@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. 👍

from gloo.