solidjs-community / solid-primitives

A library of high-quality primitives that extend SolidJS reactivity.
https://primitives.solidjs.community
MIT License
1.23k stars 122 forks source link

why not delegated events? #348

Open JacobZwang opened 1 year ago

JacobZwang commented 1 year ago

When using Solid's native handlers such as onClick(), events are delegated. Handlers created in this library do not use delegated events and instead opts for native handlers. That means that if I were to call e.stopPropagation() inside a delegated event, it wold not perform as expected. The native listeners registered by this library would fire first even if they are watching a parent on the child who called e.stopPropagation(). I was able to solve this problem by creating my own makeDelegatedEventListener util.

import { addEventListener, delegateEvents } from "solid-js/web";

export const makeDelegatedEventListener = (
    node: Element,
    name: string,
    callback: (e: any) => void
) => {
    addEventListener(node, name, callback, true);
    delegateEvents([name]);
};

This makes bubbling work as expected. Why does this library not also delegate events or why does it not at least provide an option to? My quick implementation above may have flaws I'm unaware of since I made it by inspecting solid's source code, but at least on the surface it seems to work as I'd expect listeners to work, whereas all listeners created by this library do not.

thetarnav commented 1 year ago

If you are asking about the reason, then that is the purpose of the event-listener package. To give a reactive interface for adding event listeners to elements. Depending on what you assume the package is doing you might expect different things. If you are looking for a familiar element.addEventListener that gets removed automatically onCleanup then native bubbling is expected.

Nonetheless, having the option to opt into event delegation is an interesting idea. I'm not sure about the implementation though. Normally delegateEvents is called immediately so that the events can be captured before the hydration is complete. Calling it asynchronously seems odd, but it works so 🤷‍♂️ Either way, I would probably match the implementation with what assignProp is doing - a function used for applying a dynamic prop at runtime when the compilation couldn't optimize. It's what the Dynamic component will use when assigning props to the HTML element. It means that an event should only be added as delegated if its name is included in the delegated set, otherwise a standard element.addEventListener is used.

If that's doable the question is: should this behavior be the default with an option to opt-out, or the opposite?

Are you interested in contributing a solution if we agree on the idea?


You can see additional discussion on the discord server: https://discord.com/channels/722131463138705510/958428791124951050/1084965305006170152

JacobZwang commented 1 year ago

Yes I was asking to see if there was a specific reason you chose not to implement this. At least for my use cases, not delegating events makes the library practically unusable. I have components like <Draggable></Draggable> that have no element themselves, and only apply listeners to their children. When a developer uses stopPropagation() lower in the tree under the draggable component, they expect it stop that event from hitting the draggable, but it doesn't, because the draggable component's events aren't delegated. This confused me for quite some time until I learned about Solid's event delegation.

Even when not using wrapper components that assign events to children, I still run into problems where stopPropagation and/or preventDefault don't work as expected. I'm actually confused why lots of other people haven't run into this problem. I'm building a fairly sophisticated 2d editor with panning, dragging, drawing, etc with many things built in html so maybe I'm just the first push solid to it's limits like this? Or maybe I'm making bad decisions in terms of my implementation? I fairly new to the solid ecosystem.

Personally I think it might be wise for event delegation to be opt in for solid as a whole because it causes weird unexpected behavior. That said, even if delegation is opt in for solid, this library still seems like it should support delegation.

I'd be happy to implement this if we can agree on a solution and it's not too difficult for me to understand the solid primitives codebase.