web-sys listener conversion

[daxpedda]

Do we really really want to support both stdweb and js-sys/web-sys? I find it far more compelling to switch completely. There are so many issues with compatibility and making a similar interface is really hard.

So I would like to propose that we just make a different branch where we can work on the web-sys migration without having to support both at the same time.

Cc #558, #826, #817, #818, #827, #841.

[jstarry]

Have you looked at using gloo? https://github.com/rustwasm/gloo/blob/master/crates/events/src/lib.rs

I would love if Yew used gloo so that we could help build out the common modules they are building

[daxpedda]

Sadly gloo really has very little that we can reuse.

Specifically the EventListener that they are implementing doesn't really work out for us because it removes the event on drop, which doesn't align with our current implementation.

[daxpedda]

I took some inspiration from gloo and was able to remove the generic.

[daxpedda]

This part is done basically, changing Listener::attach to use web_sys::Element would break almost everything, so I'm going to put this in the next PR.

• I called the feature web-sys, any other name we want to use here?
• The macro is way more complicated now, is this alright or are there any good ideas to improve this?

[jstarry]

This part is done basically, changing Listener::attach to use web_sys::Element would break almost verything, so I'm going to put this in the next PR.

• I called the feature web-sys, any other name we want to use here?
• The macro is way more complicated now, is this alright or are there any good ideas to improve this?

Awesome! Great work! This is probably the most daunting part of dual-support for stdweb / web-sys

Thanks for splitting up the work, I'm not sure if this PR is shippable though. I'm thinking it might be a good candidate for a feature branch until the bulk of the work is done. Thoughts?

I called the feature web-sys, any other name we want to use here?

web-sys is great

The macro is way more complicated now, is this alright or are there any good ideas to improve this?

More macros! 😆 But actually... 😉 I think some extra macro nesting could clean up the implementation / repetition. I'm cool with that coming later

[jstarry]

Sadly gloo really has very little that we can reuse.

Specifically the EventListener that they are implementing doesn't really work out for us because it removes the event on drop, which doesn't align with our current implementation.

Ah that's a bummer, could you add an internal comment in the code to explain in detail why we can't use gloo here? I imagine as gloo gains more momentum there will be calls for Yew to integrate it

[daxpedda]

I'm thinking it might be a good candidate for a feature branch until the bulk of the work is done. Thoughts?

Agreed.

I think some extra macro nesting could clean up the implementation / repetition. I'm cool with that coming later

There are some more issues that surfaced, I will most likely split the implementation into two files now 😞.

[jstarry]

There are some more issues that surfaced, I will most likely split the implementation into two files now 😞.

No worries! Happy you're taking the time to get it right 👍

[jstarry]

A cleaner solution?

Yes, much cleaner!

[daxpedda]

I was trying to abstract the Wrapper struct away, but apparently rust isn't very happy about macro invocation inside a macro, so this is the cleanest I'm getting.

[daxpedda]

Just had an idea about how to solve that macro, I think this is as good as I will get it.

[daxpedda]

Moved it to the new web-sys branch.

[jstarry]

Sorry for the delay, want to give this a closer look still. Holidays have been busy 😪

[richard-uk1]

Specifically the EventListener that they are implementing doesn't really work out for us because it removes the event on drop, which doesn't align with our current implementation.

The idea with gloo::events is that you can either use the RAII handle to avoid leaking memory, or call .forget() on the EventListener, which leaks the memory intentionally.

[daxpedda]

The idea with gloo::events is that you can either use the RAII handle to avoid leaking memory, or call .forget() on the EventListener, which leaks the memory intentionally.

Aha! I didn't see that it had it's own forget implementation, I wanted to leak as little as possible.
Will fix this soon.

[daxpedda]

Done.

[jstarry]

Specifically the EventListener that they are implementing doesn't really work out for us because it removes the event on drop, which doesn't align with our current implementation.

I'm not sure there's value in keeping consistent with our current implementation. I would rather that all event listeners get removed on drop. We don't expose event listener handles to the user right now anyways. Thoughts?

I think we could wrap the stdweb event listener handle so that we can remove on drop like we do here: https://github.com/yewstack/yew/blob/master/src/services/keyboard.rs#L88

[daxpedda]

Sounds good, I will be free in a couple of hours to do just that.

[daxpedda]

Done.

[jstarry]

@daxpedda this requires a change to vtag.rs as well, it's still calling .remove() on the handlers. I think it's best to pull that change out of this PR and merge to master.

[daxpedda]

I already fixed that in the latest commit of #817.
Right? I hope I'm not missing something here.

[jstarry]

I already fixed that in the latest commit of #817.

Ah ok, that's fine