solid icon indicating copy to clipboard operation
solid copied to clipboard

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

Open KrofDrakula opened this issue 3 years ago • 19 comments

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?

KrofDrakula avatar Jan 22 '22 10:01 KrofDrakula

You need to extend

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

atk avatar Jan 22 '22 10:01 atk

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.

KrofDrakula avatar Jan 22 '22 11:01 KrofDrakula

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

atk avatar Jan 22 '22 12:01 atk

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.

KrofDrakula avatar Jan 22 '22 12:01 KrofDrakula

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.

otonashixav avatar Jan 22 '22 12:01 otonashixav

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.

ryansolid avatar Jan 22 '22 15:01 ryansolid

~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 avatar Jan 22 '22 18:01 KrofDrakula

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.

KrofDrakula avatar Jan 22 '22 18:01 KrofDrakula

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.

ryansolid avatar Jan 22 '22 19:01 ryansolid

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.

KrofDrakula avatar Jan 22 '22 20:01 KrofDrakula

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.

ryansolid avatar Jan 23 '22 03:01 ryansolid

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.

KrofDrakula avatar Jan 23 '22 06:01 KrofDrakula

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.

martinpengellyphillips avatar Feb 01 '22 20:02 martinpengellyphillips

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.

ryansolid avatar Feb 01 '22 21:02 ryansolid

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.

martinpengellyphillips avatar Feb 01 '22 21:02 martinpengellyphillips

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.

KrofDrakula avatar Feb 02 '22 07:02 KrofDrakula

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.

marvinhagemeister avatar Feb 08 '22 12:02 marvinhagemeister

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

mendrik avatar Mar 18 '22 11:03 mendrik

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 avatar Mar 18 '22 14:03 ryansolid

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.

ryansolid avatar Dec 20 '22 21:12 ryansolid