[WIP] gloo-events

[Pauan]

Do not merge yet!

Since the current plan is to create a high-level statically typed events system, what should we do with this mid-level API?

I think it makes sense for the high-level and mid-level API to both go into the gloo-events crate, but should we put EventListener into a submodule? Or is it okay to leave it at the root? Will that confuse users?

Fixes #30

[Pauan]

Also, what name should we use for the string for the event type? Right now I call it kind (since type cannot be used), but perhaps tag would be better? Or maybe something more verbose like event_type?

EDIT: I went ahead and renamed it to event_type, since I think that's the clearest.

[Pauan]

Also, right now it only accepts &str. What if we want to allow for passing in a String into the EventListener (and have it be owned by the EventListener)? That can be useful for strings which are dynamically created.

[fitzgen]

I think it makes sense for the high-level and mid-level API to both go into the gloo-events crate, but should we put EventListener into a submodule? Or is it okay to leave it at the root? Will that confuse users?

I think that is fine, since the high-level, static API doesn't handle dynamic event listening, so it makes sense to have both, IMO.

[fitzgen]

Looking good -- just need to get all the docs in place, rustfmt, and double check each of the check list items over here: https://github.com/rustwasm/gloo/blob/master/CONTRIBUTING.md#gloo-crate-checklist

Thanks @Pauan!

[Pauan]

@fitzgen Before I can fix this up, I think we need to resolve these two issues:

#30 (comment)
#42 (comment)

[fitzgen]

Also, right now it only accepts &str. What if we want to allow for passing in a String into the EventListener (and have it be owned by the EventListener)? That can be useful for strings which are dynamically created.

Sounds like a perfect opportunity to use Cow<str>

[Pauan]

@fitzgen I don't think that works, since Cow has a lifetime, which is attached to the lifetime of the original String (so it's not any better than just using &'a str directly).

The goal is to be able to dynamically create a String, and then shift ownership of the String into the EventListener (so that way the String will get cleaned up at the same time as the EventListener).

[fitzgen]

I don't think that works, since Cow has a lifetime, which is attached to the lifetime of the original String (so it's not any better than just using &'a str directly).

The lifetime can be 'static (optionally; can also be 'a for short-term listeners) which would enable usage like this:

// assuming this signature:
impl<'a> EventListener<'a> {
    pub fn new<S, F>(target: &EventTarget, event_type: S, callback: F) -> Self
    where
        S: Into<Cow<'a, str>>,
        F: FnMut(Event) + 'static,
    { ... }
}

// Enables both
EventListener::new(&target, "click", |event| { ... });

// and
let s: String = ...;
EventListener::new(&target, s, |event| { ... });

We could remove the <'a> and require S: Into<Cow<'static, str>>, since stack-lifetime event listeners aren't really a thing. Then the event type would be either a &'static str or a String (no &'a strs).

The other option would be adding the S type parameter to EventListener itself, but I think that is a bit much here: we don't need the monomorphization for perf reasons, and it would make the API that much more confusing, and compile times take that much longer.

[Pauan]

@fitzgen Interesting, I hadn't seen that Into<Cow<...>> pattern before.

That does mean that all dynamic event types must be String, it wouldn't be possible to use &'a str anymore.

But as you say, allowing for &'a str is a little weird, since it's basically saying that the string is stack allocated but the EventListener wouldn't be.

[Pauan]

Something new I just discovered:

https://developer.mozilla.org/en-US/docs/Web/API/EventTarget/addEventListener#Improving_scrolling_performance_with_passive_listeners

It is apparently possible to significantly improve the performance of the browser by setting passive: true on the event listeners.

The only downside is that now you cannot call event.preventDefault().

My thinking is that we should make passive: true the default, and provide a way to manually set passive: false if the user needs to call event.preventDefault().

@fitzgen Any thoughts on this?

[fitzgen]

I think that the mid-level API probably shouldn't be as opinionated as that, but maybe the higher-level API could be.

I think we want an EventListener constructor that takes a web_sys::AddEventListenerOptions reference either way, so that users can control all this.

Orthogonal to the expressive ability to define event listener options: As another layer, the web_sys::AddEventListenerOptions builder API isn't too bad, but I think we can do better than allocating the same, fresh objects (e.g. { passive: true }) over and over every time we add event listeners. There are only three boolean options on the builder, so only eight different combinations. We could define every combination up front, store them in an array. They would be indexed by the binary number for whether each option was set or not. This way, we could re-use the JS objects, instead of re-allocating them every time we add an event listener.

[Pauan]

I think that the mid-level API probably shouldn't be as opinionated as that, but maybe the higher-level API could be.

To be clear, we're just discussing the defaults. Right now the browsers will default to passive: true for some events, but not others.

The reason the browsers have (started to) default to true is because 99+% of event listeners don't need to use event.preventDefault(), and if there is even a single event listener with passive: false it can significantly slow down the entire web page, simply because the event listener exists.

The only reason the browsers aren't always defaulting to true is because of backwards compatibility. But we aren't limited by backwards compatibility.

What you're suggesting is essentially that the easy way should incur significant performance costs (even if the event listener doesn't actually use event.preventDefault()), and the user has to go out of their way to use the fast version.

What I'm suggesting is that the easy way should be fast, and that the user should go out of their way to enable event.preventDefault() if and only if they actually need it. I think that is both more intuitive and fits in perfectly with Rust's overall philosophy of making performance costs explicit.

I think we want an EventListener constructor that takes a web_sys::AddEventListenerOptions reference either way, so that users can control all this.

Agreed.

As another layer, the web_sys::AddEventListenerOptions builder API isn't too bad, but I think we can do better than allocating the same, fresh objects (e.g. { passive: true }) over and over every time we add event listeners.

Also agreed, I think that's a good idea.

[fitzgen]

What I'm suggesting is that the easy way should be fast, and that the user should go out of their way to enable event.preventDefault() if and only if they actually need it. I think that is both more intuitive and fits in perfectly with Rust's overall philosophy of making performance costs explicit.

Ok, I'm convinced.

[Pauan]

@fitzgen Okay, great, I think that nails down all the design elements, so I'll try to get this finished ASAP, since I know it's blocking other work.

[Pauan]

@fitzgen I fixed all the issues that were brought up.

I'm very happy with both the API and the implementation, but I want to make sure we have consensus that this is the right public API before I do the hard work of adding in docs and unit tests.

In particular, I'm not so sure about EventListenerOptions::to_js

Implementation-wise, since new and once are going to be used 99%+ of the time, I created thread-locals for those, but not for the other 6 combinations of EventListenerOptions (this can be improved later, or users can manually cache if they wish).

[fitzgen]

API looks good! Let's get those docs and tests written :)

[Pauan]

Just to clarify the status of this: I just need to put crate-level docs and add some docs to the guide and this'll be ready for review.

[Pauan]

Okay, this is basically done, and ready for the final review.

I made some API changes since the previous review:

• Rather than using a bool for capture/bubble, I instead created an actual EventListenerPhase enum. I think this makes the API much clearer for users.

• I added in Default impls to EventListenerOptions and EventListenerPhase

• I added in some convenience methods to EventListenerOptions for common situations (capture, enabling preventDefault)

I added in unit tests, and docs for everything, but I wasn't sure what to put into the Gloo Guide... the other crates have an empty entry in the Guide, and wouldn't it just be repeating the crate-level docs...?

[Pauan]

(By the way, my intention is to make the change from FnMut(Event) to FnMut(&Event) in a separate PR, since we have to wait for a wasm-bindgen release for that)

[fitzgen]

This looks great! Most of my feedback is just documentation wording nitpicks :-p

The one more tangible suggestion is that I think we should have a test that event listeners are removed on drop properly:

• add event listener A with function F
• add event listener B with function G
• fire event
• assert both F and G observed the event
• remove listener A
• fire event
• assert F observed the event and G did not
• add event listener C with function G
• fire event
• assert both F and G observed the event

Thanks @Pauan!

[fitzgen]

We could add raw getters for the underlying closure's js_sys::Function, and then use web-sys manually to remove the listener. This doesn't clean up the Rust closure, however. Probably that's alright (particularly since it isn't actually closing over anything, and therefore should just be a fn pointer).

[Pauan]

and then use web-sys manually to remove the listener.

Since it's attached to a <div> element which doesn't exist in the DOM, the event listeners themself will already be automatically cleaned up (by the browser and JS's GC).

So the comment was just about the Rust Closure memory.

[Pauan]

@fitzgen I debated whether to post this in the RFC thread, but since it's a (relatively) small change I decided to just go ahead and do it.

The once option in EventListenerOptions has bugged me for a while, and after a lot of thinking I came up with a solution:

1. I removed once from EventListenerOptions.
2. I added a new once_with_options method, which is just like new_with_options except it accepts FnOnce instead of FnMut.

I'm quite happy with this, now there is a nice symmetry:

• new -> new_with_options
• once -> once_with_options

And the EventListenerOptions can be used with either new_with_options or once_with_options.

I think this makes the API incredibly clear for users (it's much simpler now), and it also now allows for using FnOnce with options (that wasn't possible before).

(I also fixed the suggestions and added some more unit tests, so this is ready for the final final review.)

[fitzgen]

Everything looks good modulo one comment below about once_with_options

[fitzgen]

Ah ok, I see now. Apologies my last review was submitted quickly before I had to run off on an errand.

If this was a public API, I'd advocate for replacing the bool with a more meaningful enum, but it isn't pub so I think the bool is fine.

[fitzgen]

I think this is ready to merge now? Am I missing anything that makes it still [WIP] as the title implies?