tsx-dom icon indicating copy to clipboard operation
tsx-dom copied to clipboard

Feedback needed for breaking changes of next version

Open Lusito opened this issue 10 months ago • 9 comments

I'm currently working on a bigger change and thought it would be a good opportunity to ask for some feedback.

Feel free to discuss these and maybe add other improvement ideas.

1. Prop Names

Prop Names are currently a mix of camelCase and lowercase. For example srcset, tabIndex and referrerPolicy. The first two are HTML attributes, which are lowercase in the spec, but the latter is a property (not set via HTML), so it's camelCase.

Event Names are on another level: while they are technically available as HTML attributes, those are specified as strings on HTML, which is not what we want here, but internally, we always convert the names to lowercase when using addEventListener. The camelCase is currently needed though, in order to detect what props are events. This might change with the folling section though. See below.

As you can see, there is some inconsistency here and this should be cleaned up. I have 3 Options to choose from:

  1. Making everything adhere to the spec, i.E. HTML attributes are all lowercase, while properties can be uppercase.
  2. Making everything camelCase for better readability. After all, this is not HTML, it is JSX (<div /> is not valid HTML for example).

2. Event Listeners

Events are currently specified like this: onMyEvent or onMyEventCapture. This was made in order to be more like react, but I'm not sure that's a good idea. It creates certain issues:

  1. We need a way to differentiate between plain values (for attributes) and events. Currently this is done by using a Regex /^on\p{Lu}/u, which is ugly.
  2. We need a way to add a modifier like "Capture". But we can currently only do capture and not any of the 2 other modifiers addEventListener provides: passive and once. There is also a conflict with event names that end in Capture like onGotPointerCapture.

I'm thinking of changing this more to be like Vues approach. I.e. @click.capture, etc. In that case, event names could be all lowercase again like in the spec.

My question here is what values to allow:

  1. Vue allows changing the order of modifiers and that matters for vue in some cases (as they have other modifiers than just the 3 from addEventListener). I'm leaning to having a fixed order. For example once, then passive, then capture. I.e. you could do @click.once.capture and @click.capture.passive, but not @click.passive.once. This has the benefit of a bit of enforced code-style, while reducing the amount of possible properties on a JSX elements a lot!
  2. I'm leaning towards not adopting the vue modifiers as well, like stop, prevent and self, since they are not part of the addEventListener call and they could probably be implemented by the user. Adding them within tsx-dom would make things more complicated, especially if order is supposed to be important.

edit: It seems, that @ is not a valid character to start a JSX prop with and . may not appear in the prop name either, so I'm looking for alternative characters:

  • It seems I can use :, but only exactly once (probably since it resembles namespaces).
    • Autocomplete/intellisense doesn't seem to work nicely on : either, so a on: prefix is not a good option.
  • A dash can appear multiple times.
    • But this makes typescript forget about type-checking.
  • $ and _ can appear multiple times without breaking anything.
    • So maybe something like $click_once_capture?

Lusito avatar Feb 16 '25 19:02 Lusito

I haven't thought deeply about potential pitfalls and/or technical issues so this is just my general gut reaction. I'm definitely supportive of consistency and not breaking expectations, however that is best achieved.

Prop Names: I'm primarily in favor of matching the HTML spec where possible. I could also get behind a decision to consistently match React's JSX conventions as it would facilitate migrating to React if a project outgrows tsx-dom in the future (though I feel like migration would be sufficiently trivial either way).

Event Listeners: From what I can tell the React team chose camel case for on* in JSX because they're wrapping the native events as synthetic events. Once again, I think I prefer sticking with lowercase onclick but would still be fine with following React conventions.

As for the modifiers like capture... I wonder if something along the lines of onclick={listener} onclick={{listener, options}} might be viable? I'm not familiar with implementation under the hood, just thinking about it terms of mostly being a wrapper around addEventListener. Adopting an approach like this might raise the argument to stick with camel case onClick since it diverges from native onclick.

pglewis avatar Feb 17 '25 17:02 pglewis

About migrating to React Not sure if that is so easy, if you are actually doing something with the dom elements, but I guess at least the JSX might stay quite similar.

Event Handlers I like the idea of removing the modifiers from the JSX itself, as it makes intellisense less overwhelming.

However, while your example of onclick={{listener, options}} looks nice like this, that's probably not how you're going to write it in a real scenario. It will probably be more like onclick={{ listener: (e) => ..., capture: true }} or, if you have your listeners separate, they'll be named functions: onclick={{ listener: onButtonClick, capture: true }}

A tuple might be a bit more compact: onclick={[onButtonClick, { capture: true }]}. It could even mimic the behavior or addEventlistener, by allowing the second part to be true as a shorthand for { capture: true }. Obviously, you could also supply the function itself without modifiers.

A different approach might be to provide helper functions, which wrap the actual listener. Something like: onclick={once(onClick)} or listen(onClick).capture? The helper functions would just add some properties to the function, which can be read by tsx-dom.

If attaching properties would be an option, this might also be an idea:

function MyComponent() {
    function onClick(event: MouseEvent) {
        // todo
    }
    onClick.capture = true;

    return <button onClick={onClick}>submit</button>;
}

Or with type helpers and arrow functions:

function MyComponent() {
    const onClick: EventHandler<HTMLButtonEvent, MouseEvent>(event) {
        // todo
    }
    onClick.capture = true;

    return <button onClick={onClick}>submit</button>;
}

Lusito avatar Feb 17 '25 18:02 Lusito

BTW, plain onclick (without camelcase) would be more in line with the HTML spec, but I would have no way of knowing if it's a listener aside from checking for the type of the value. I guess that might be a way to do this, but I wonder if there are situations, where this might cause issues.

Lusito avatar Feb 17 '25 18:02 Lusito

An additional issue, I found while working on this: There are two properies, which fall a bit out of normal bounds:

  • <meta http-equiv="..." />
  • <form accept-charset="...">

Both use a dash-case property in HTML, but camelCase in React and tsx-dom. I actually think they are probably broken right now in tsx-dom, as they would set properties "httpequiv" and "acceptcharset" due to them being typed as "httpEquiv" and "acceptCharset"

The dash-case has the problem, that you can easily have a typo and not get notified by it: <meta htp-quiv="..." /> would not complain.

Lusito avatar Feb 17 '25 18:02 Lusito

BTW, plain onclick (without camelcase) would be more in line with the HTML spec, but I would have no way of knowing if it's a listener aside from checking for the type of the value. I guess that might be a way to do this, but I wonder if there are situations, where this might cause issues.

I have been (probably naively) working under the assumption that an exhaustive list of valid HTML event names could be checked.

pglewis avatar Feb 17 '25 18:02 pglewis

It's not that easy, as users can define their own.

But I think that a check for typeof value === "function" && name.startsWith("on") should be enough. Anything else would result in setAttribute being called with a function as value, which just stringifies it, which doesn't seem like anything anyone would want.

Lusito avatar Feb 17 '25 19:02 Lusito

Just tried this: <button onclick={Object.assign(onClick, { capture: true })} /> works for the property assignment approach as well. Though not with Intellisense and it doesn't complain about typos either. But it does complain if onClick has invalid parameter types.

Lusito avatar Feb 17 '25 19:02 Lusito

I've just pushed the next branch with my current progress.

  • Intrinsic elements have been rewritten, so that you should get accurate properties and types.
  • I've added the approach of assigning properties to the listener function just to try it.
  • I'm keeping react-ish camelCase for now, since it makes the transition for users less painfull and with the new approach of not having modifiers like capture in the JSX syntax, it's not needed anymore to try something different.

I'll let this sit here for a while for others to comment on before finalizing it.

If you want to try it, the easiest way is just to checkout the branch and use a test file like packages/tsx-dom/src/base.spec.tsx to experiment with the "new JSX"

Lusito avatar Feb 17 '25 20:02 Lusito

I'll give it look this evening

pglewis avatar Feb 17 '25 20:02 pglewis