solidjs / solid

A declarative, efficient, and flexible JavaScript library for building user interfaces.
https://solidjs.com
MIT License
32.41k stars 923 forks source link

Solid JSX HTMLElement types do not allow `on:event` props #804

Closed KrofDrakula closed 1 year ago

KrofDrakula commented 2 years ago

Describe the bug When using the on:event syntax on HTML elements in JSX in TypeScript the props are considered invalid:

<div on:click={() => null} />
(JSX attribute) on:click: () => null
Type '{ "on:click": () => null; }' is not assignable to type 'HTMLAttributes<HTMLDivElement>'.
  Property 'on:click' does not exist on type 'HTMLAttributes<HTMLDivElement>'. Did you mean 'onclick'?ts(2322)

Expected behavior

The behaviour of on:[event] handlers should be typed correctly. I've tried extending the HTMLElement interface to allow this, but I'm stuck with being unable to correctly:

interface HTMLElement {
  [prop: `on:${keyof HTMLElementEventMap}`]: <
    T extends keyof HTMLElementEventMap
  >(
    ev: HTMLElementEventMap[T]
  ) => void;
}

Unfortunately, the error with this type seems to suggest that I'd have to write out each individual on:${event} JSX handler, unless someone can suggest how to create a mapped type from the keys presented above. Before I attempt that, would this type extension belong in solidjs or dom-expressions?

atk commented 2 years ago

You need to extend

declare module "solid-js" {
  namespace JSX {
    interface CustomEvents {
      click: (ev: MouseEvent) => void;
    }
  }
}
KrofDrakula commented 2 years ago

Right, good point, I was looking at the wrong type. Where would that belong if I were to submit a PR for this here? I couldn't find any JSX type declarations in the source code here.

atk commented 2 years ago

You misunderstand: this goes into your typescript file. Using on: for HTMLElement Events is considered wasteful, since Solid.js already exposes deferred events to improve performance.

The on: API is solely meant to handle custom events that you dispatch yourself (or a library you use).

KrofDrakula commented 2 years ago

The problem with deferred events in my case is that I rely on stopping propagation on components (like <input>) that should prevent ancestor event handlers from receiving events.

In Solid's case, if I attach an event handler on the document and another deferred event handler on a child component, the parent's handler will be triggered first, contrary to assigning the event handler on the child which stops the propagation. It's impossible for the child to prevent the parent from receving the event, unless I also keep track of various signals that combined give information in the parent event handler to ignore the event.

As far as I'm aware, this mechanism in Solid is available for just such cases, and the technique is something the DOM provides and is a valid use case.

otonashixav commented 2 years ago

You can add the missing handlers via a mapped type like so: https://playground.solidjs.com/?hash=300022548&version=1.3.3

The JSX types are declared in dom-expressions; if they are to be changed to include on: handlers it's a good opportunity to DRY the types since the handlers are duplicated for lower- and upper-case variants currently.

ryansolid commented 2 years ago

If it can't prevent propagation between delegated events there is a bug or missing feature. My expectation is this should work. It is possible for a non-delegated event to be hit first, but anything in Solid won't start its bubbling until the actual event bubbles to the document and then it bubbles from the target back up again by our own mechanism and still should be cancellable.

KrofDrakula commented 2 years ago

~It looks like this example worked as expected with Solid 1.0.0, but it doesn't work in 1.3.3 at least:~ Actually, I tried rolling back to Solid 1.0.0 as that was what the initial Solid TSX starter template was, but it might have been a fluke with HMR that caused a bad result. In this codesandbox, the global handler fires first, even though it's in bubble phase:

https://codesandbox.io/s/modest-snow-1y1v9?file=/src/index.tsx

Note the use of the global document.addEventListener() which I'm using to capture global keystrokes.

KrofDrakula commented 2 years ago

Same thing in the playground:

https://playground.solidjs.com/?hash=1871281921&version=1.3.3

I might just be misunderstanding how interop code should work with Solid's delegated events, but I would expect being able to stop propagation at at least the component root node.

ryansolid commented 2 years ago

I see so the case is a delegated and non-delegated handler. In that case non-delegated tend to run first. Delegation happens at the time of the document handler. So its a matter of which was registered first I think. And stop propagation might be too late at that point. Using the custom event syntax is one choice. I wonder if there is another. Like exposing a method that would imperatively add the event and play nice.

KrofDrakula commented 2 years ago

Yeah, I tend to work on systems that need to play nice with other legacy components, so relying on precise DOM event binding is key here. I don't mind opting into the on:... syntax, but my original question was why wasn't it enabled via types and whether I'm missing something.

ryansolid commented 2 years ago

Ah.. mostly not to confuse things with different ways to add native events. I'm not a TS user so I don't really care. If people have opinions glad to hear them.

We can make on: for at least all the delegated events I figure.

KrofDrakula commented 2 years ago

So should I close this issue and write up a PR per @otonashixav's comment on dom-expressions? I don't mind doing the legwork, I just need a bit of guidance to implement it properly so that the types propagate correctly as this is the first time I'd be extending JSX types.

martinpengellyphillips commented 2 years ago

As a related note - I encounter the issue of delegated vs non-delegated event listeners when creating libraries that use ref/use: callbacks for convenience. In the callback I setup listeners using el.addEventListener, but then there is no way for callers using a typical onX Solid handler to prevent those handling the event (and it is not immediately obvious why). I'd be interested in a better approach here if there is one.

ryansolid commented 2 years ago

It's possible I should just expose the helpers in a way that are more usable. There are delegated equivalents to addEventListener internally it is just they are built in such a way the caller decides whether to delegate or not. This is useful because the compiler can make this determination ahead of time instead of pulling in code to check against a list at runtime. But we could maybe expose that. In general this is always the case with delegated events in that they run after the standard event cycle.

I suppose the ideal form would have the ability to let the compiler augment elements after the fact with a JSX syntax it could apply the right things.

martinpengellyphillips commented 2 years ago

That sounds interesting to me (and I think being able to decide on the library author side whether to delegate or not would be useful). Let me know if there is something now/in future I can try out for my use case and report back.

KrofDrakula commented 2 years ago

I would definitely like the documentation and tools to describe what the idiomatic way of opting into/out of delegated events would look like in Solid, and if there is delegation happening, which events are affected by this. I can see a lot of other developers coming from Preact and similar libraries being caught out by this if they're used to leveraging the web platform's native event affordances.

marvinhagemeister commented 2 years ago

I can see a lot of other developers coming from Preact and similar libraries being caught out by this if they're used to leveraging the web platform's native event affordances.

Just an FYI: Preact doesn't do event delegation. It's similar to solidjs in how browser events are handled.

mendrik commented 2 years ago

do deferred events bubble up? if so I would rather just add another on:keydown farther up in my app if I needed a global keyevent handler

ryansolid commented 2 years ago

do deferred events bubble up? if so I would rather just add another on:keydown farther up in my app if I needed a global keyevent handler

If you mean delegated events, they bubble but do so with custom bubbling that happens when the original event has already bubbled up to the document.

ryansolid commented 1 year ago

I'm not TS savvy to know what the preferred choice is here. But this issue is about as stale as it gets and no one commented on whether we should do this.

I think that it is interesting whether we should basically use types to direct people to the overload as it isn't the intended usage but I can see why people would want these types if they used them all the time. In either case we can probably take this to DOM Expressions or at least a discussion.