Enhance typing in vanjs-core and vanjs-jsx

[yahia-berashish]

This PR fixes #142
It modifies the Tags and Props in the core.
And uses the modified types in VanJSX.

[Tao-VanJS]

Thanks so much for your PR! A few initial take aways:

1. I will appreciate if you can make the PR in a way that doesn't change too much of the formatting of the van.d.ts file. Otherwise it's really hard for me to see clearly what are being changed in this particular PR. Also, I prefer not to make the overall coding style inconsistent across files.
2. HTMLElementTagNameMap is really a good one. I didn't know it before, but looks like it can simplify van.d.ts a lot.
3. I probably need more time to think through whether the property changes are desirable. In VanJS, props can either be a property value of the UI element, or an HTMLAttribute. I am not seeing in this PR, HTMLAtrribute being particularly handled. On the other hand, I don't know a concrete use case where the current definition of Props has a big issue. If you know one, I will appreciate if you can let me know. Furthermore, I really need to think through whether it can introduce major breaking changes (at least I need to check all the existing 1st party TypeScript files can continue to work with the new type definition).
4. Probably it will be more convenient to separate the changes between van.d.ts and the changes in van-jsx add-on into different PRs. These changes will likely go through separate release processes. And also if Step 3 needs more time, I am also open to getting the changes regarding HTMLElementTagNameMap released first.

[yahia-berashish]

Of course, take your time @Tao-VanJS , the changes in the PR are relatively small scale changing only two files but they can change a lot of the typing support for VanJS.

I don't seem to understand what you mean by HTMLAttribute, the changes in van.d.ts change the approach from manually declaring the attributes to using the HTMLElementTagNameMap and modifying the values to include states and binding functions.

The issue with the Props type was that it added support only for the Element type attributes, which is more generic than the HTMLEelement or SVGElement types, so you can't find the value property in an input() tag for example, yet the Props type allows you to add additional types without type-checking them, so you can use a number or an object for the input's value property and typescript won't warn you, which defies the point of using it in the first place.
the latest changes make it so the element function will only accept the right types for attributes, while also accepting states and binding functions, giving us a full solution.

Let me know if there are any code inconsistencies you would like me to take notice of, and you are probably right about separating the two commits, will stash one of them and make a new PR soon.

Note: The van.debug.d.ts wasn't modified, will work on it later if needed.

[Tao-VanJS]

First, about code style changes introduced by this PR. For instance, I've noticed the changes like that:

I understand the code style changes might be introduced by some auto-formatting tools. But the code style of VanJS is currently maintained manually. Using auto-formatting tools will introduce lots of code style changes that make the real changes of this PR hard to see. It will also introduce inconsistent coding styles across different files.

Second, about the type system changes in this PR, I cloned your fork. And I found lots of typing errors just in the test file. I guess there will probably be a long way to go if we ever want this PR doesn't break any existing code.

Specifically, regarding to the comments about HTMLAttribute, let's say for instance, I want to build an element like this:

<span data-index="1"></span>

In the current version of VanJS, it's very easy to build it with the code:

span({"data-index": 1})

But I guess there will be type errors if the current PR is applied.

Last but not least, in the current version of van.d.ts, we do have some type-checking for property values. It only allows mostly primitive types for property values, thus trying to assigning an object to value property will anyway result in a type error. Trying to assign a number to value property won't result in a type error. But IMHO, this might be the desirable behavior, as sometimes we probably do want a numeric value for certain properties.

[yahia-berashish]

What I understand is that -excluding the code style, which was caused by prettier and possibly by my usual coding style in the rest of my projects- the issues are mostly because the typing is too strict, types like class and data- attributes are not allowed with the changes introduced by the PR.
I think a good middle ground will be where the element props are mainly the modified props of the HTMLElement, providing type recommendations and autocompletion while making additional props possible to add without the need to skip the type checks.
I think the current implementation is too loosely typed, both for the core and the addon (in addition to that the JSX addon has typing issues), the one proposed by the PR might be too strict, probably because I come from React which enforces the types in a sophisticated way.
Might I get recommendations on how you would like the types' interface & API to look?
PS: Excited about VanX!

[Tao-VanJS]

I think in React, it also supports specifying arbitrary attributes in Props, right? If so, I would be curious to learn how React enables a stricter type checking and support arbitrary attributes at the same time. Possibly there might be things we an learn from.

Regarding to how to proceed with the improvement ideas brought up by this PR, I am suggesting to break down the problem into pieces and tackle them piece by piece:

1. It's definitely beneficial to introduce HTMLElementTagNameMap, as it can simplify van.d.ts a lot. This part can be done immediately.
2. There might be other incremental improvement we can make to van.d.ts without too much risk of breaking existing code. For instance, it should be pretty safe to exclude functions from valid property values for properties that don't start with on.... That's probably something we can do without too much worries.
3. Other than that, changes to van.d.ts need to be more careful. Personally I prefer to see changes being made in an incremental way:
   1. If there is a specific use case where VanJS's type system is too strict (i.e.: code of a legitimate use case resulting type errors), I would like to learn the concrete use case and see what changes can be made to address it.
   2. If there is a specific use case where VanJS's type system is too loose (obviously problematic code that couldn't be caught by type checking), I would like to learn the concrete use case and see what changes can be made to address it.
4. Without effecting the type-checking, I think we can also make improvement for TypeScript's auto-completion support. For instance, for Props type, we can also define ad-hoc fields in addition to inheriting from Record<string, PropValue | StateView<PropValue> | (() => PropValue)>. The type of ad-hoc fields can be kept the same as others (i.e.: just PropValue | StateView<PropValue> | (() => PropValue) for now) so that these ad-hoc fields won't change any type-checking logics. But they provide better auto-completion in TypeScript files.

I am planning a new release of VanJS soon with a couple minor improvements in my mind. I am considering to include things that are easy to implement from the list above in the upcoming release.

Please let me know if the proposal looks good to you.

[Tao-VanJS]

Some updates:

It's definitely beneficial to introduce HTMLElementTagNameMap, as it can simplify van.d.ts a lot. This part can be done immediately.

This is already committed to the 1.2.2 branch.

There might be other incremental improvement we can make to van.d.ts without too much risk of breaking existing code. For instance, it should be pretty safe to exclude functions from valid property values for properties that don't start with on.... That's probably something we can do without too much worries.

I tried to implement this, but it didn't seem to work as I hoped. Specifically, I tried to change the type definition of Props to:

export type PropValue<K> = K extends `on${infer _}` ?
  Primitive | ((e: any) => void) | null :
  Primitive | null

export type Props = {
  [K in string]: PropValue<K> | StateView<PropValue<K>> | (() => PropValue<K>)
}

But {onclick: () => {}} doesn't seem to be compatible with type Props. For property onclick, it doesn't pick up the type definition Primitive | ((e: any) => void) | null as I hoped. I don't know exactly why. Maybe this is a result of the sophisticated type gymnastics in TypeScript.

Without effecting the type-checking, I think we can also make improvement for TypeScript's auto-completion support.

Support for auto-completion also seems hard to add. Think about a regular workflow, where a user typically writes:

div(
  {
    id: "some-id",
    class: "some-class",
  },
  <children>,
)

When the user specify the Props inside this code snippet, I don't think TypeScript would have enough information to know the user is trying to edit the Props for an HTMLDivElement. Thus I guess TypeScript just can't help with the auto-completion as we would have hoped.

---

In summary, there probably isn't too much we can do in the short term other than using HTMLElementTagNameMap to simplify the code, which is already done.

[yahia-berashish]

Agree 👍, will try to make smaller changes in the future, but I'm happy nonetheless to give new insight.
The typing issues in vans_jsx aren't solved yet though.
I say we close this PR, I will open a new one where I try to address the JSX typing issues, it shouldn't be too complicated, just adding support for states and binding functions as props & children, it is urgent since VanJSX cannot be really used with TypeScript in its current state.

And about typing in React, I was talking about the HTML JSX elements rather than functional components, in React, an element like a div or an input will provide auto-completion for its props, and will trigger a typescript error when a non-existing attribute is added as long as it doesn't start with data- or aria-, and has type checking for individual attributes apparently instead of a general one like the VanJS implementation (e.g. VanJS allows for a boolean to be used as an input value, React doesn't), but also keep in mind that React can rely on the default types provided by the DOM more than VanJS since it doesn't use binding functions or a state object to handle reactive attributes.

[Tao-VanJS]

@all-contributors please add @yahia-berashish for ideas

[allcontributors]

@Tao-VanJS

I've put up a pull request to add @yahia-berashish! 🎉

[Tao-VanJS]

Without effecting the type-checking, I think we can also make improvement for TypeScript's auto-completion support.

FYI, in VanJS 1.2.5 release, we have enabled auto-completion support for props in tag functions.

[yahia-berashish]

Great, will continue to work on #142 to fix type issues in VanJSX.