Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

update types #380

Open
wants to merge 24 commits into
base: main
Choose a base branch
from
Open

update types #380

wants to merge 24 commits into from

Conversation

titoBouzout
Copy link
Contributor

@titoBouzout titoBouzout commented Nov 2, 2024

Couple of changes and updates to typings:

  • Marks as @deprecated any attribute using camelCase in case-insensitive areas, such HTML. (made sure there's a lowercase version for the attributes changed)
  • added couple of missing attributes
  • updated a couple of interfaces to its versions, example using HTMLQuoteElement instead of HTMLElement
  • added MathML typings (this mostly comes from Preact)
  • added WebView
  • deprecate classList with message Use "clsx" instead. https://www.npmjs.com/package/clsx
  • update some events
  • misc values updates and improvements
  • add links to MDN for tags
  • remove some invalid fields
  • add obsolete fields, the reason for this is that we will be able to quickly see if a field is missing, if the field is there but marked as @deprecated we avoid having to open mdn to see if there was a reason for the field to be missing. It would also play nicely with old html.

This issue can be closed solidjs/solid#2049

Typings have been compared with other frameworks via this table https://github.com/potahtml/namespace-jsx-project/blob/master/jsx/readme.md

Related:
https://discord.com/channels/722131463138705510/783844647528955931/1301945792059277333
https://discord.com/channels/722131463138705510/1301968809489600552

I haven't deprecated classList, shall we?

@titoBouzout titoBouzout marked this pull request as draft November 3, 2024 18:14
@titoBouzout titoBouzout marked this pull request as ready for review November 5, 2024 14:19
@ryansolid
Copy link
Owner

ryansolid commented Nov 7, 2024

The only thing I'm thinking is does the camelcase really matter in the case insensitive scenario. I didn't realize we had so many but also it doesn't really matter. We can still inline them in any case without transforming and they still work. It makes moving from React a lot easier which is worth considering when it doesn't cost us anything mechanical. Then again deprecated could be the right handling of this.

A lot of people seem to like writing them camelcase for readability. Although I imagine places where it matters would be confusing.. like we've never supported aria- attributes as camelCase.

In any case I want to make the decision here sooner than later because this is a big PR and I don't want you to have to wrangle with types as we constantly get new type updates.

And I imagine we will have to mirror this on the other jsx types too..

@titoBouzout
Copy link
Contributor Author

Thanks, good question. My main consideration is that we want to inline in the template as much as possible. We would like to make people think of the keys as attributes, and only resort to properties when it's really a property. This works better for template cloning, SSR(browsing rendering the response), and avoiding hydration of things that don't need it.

What I am trying to attempt with this change is for people to start writing <video disablepictureinpicture="true"/> instead of <video disablePictureInPicture={true}/>, the last one will be compiled to a setAttribute/setProperty. (with dynamics there isn't a choice)

This is a value problem not a case problem, but it reflects the problem. Even if we ignore the case and write it to the template, it would be nice to push people to think of this as attributes, write them lowercase and provide the string value.

Now that I think, the values on this PR have not been updated to reflect this intention, example accepting "true" | "false" instead of boolean. So now a question, should I update the boolean values for attributes to accept "true" | "false" | boolean ? We can solve some of this compiler side, but providing the typing for autocomplete would be better.

Additionally, I am thinking that a key with camelCase (in case-insensitive contexts, ex not svg) could be considered a property instead of an attribute. (for future-proof heuristics and make things more intuitive). Maybe even get rid of some special cased attributes (ex not having to ship an array with textContent).

About people moving from React, yeah a deprecation notice shouldn't really that bad, at least I think it's better than erroring out. Possibly we could also add the missing camelCase keys and mark them as deprecated even when these weren't supported? That table I made has been very helpful.

About the other jsx types, I haven't investigated how to do this, but it would be nice to be able to reuse one into the other.. because replication It's untenable unless enforced at merging time.

PD: remember this PR deprecates classList which I am not sure if you want to do now.

@webstrand
Copy link

Not sure if they're desired but I noticed the lack of the non-standard attribute results (but inconsistently not incremental) on InputHTMLAttributes, cf. https://developer.mozilla.org/en-US/docs/Web/HTML/Element/input/search.

@titoBouzout
Copy link
Contributor Author

Thanks, added it back. We want to support what could be used I supposed, regardless if its standard. We can mark them as non-standard, I did for some MathML attributes, but not consistently for the whole table.

@webstrand
Copy link

webstrand commented Nov 7, 2024

A lot of people seem to like writing them camelcase for readability.

I don't really care whether camelCase or flatcase, but I really want one or the other to be picked. With the types permitting either casing, components that wrap native elements have to handle both casings, which adds extra logic and is annoying.

I don't mind if this is a type-level only change. Just something to alert me to errors when a wrapper component only handles onclick but got passed an onClick prop.

flatcase matches html, in react it's somewhat annoying that you can't copy html into or out of jsx without rewriting some of the attributes. So my vote is for flatcase.

@ryansolid
Copy link
Owner

I don't really care whether camelCase or flatcase, but I really want one or the other to be picked. With the types permitting either casing, components that wrap native elements have to handle both casings, which adds extra logic and is annoying.

This is the kind of thing I want to understand. You are right trying to narrow the types yourself would be annoying as well as trying to check for all variants.

Now that I think, the values on this PR have not been updated to reflect this intention, example accepting "true" | "false" instead of boolean.

I thought we already did that for psuedo booleans like draggable. For actually boolean properties "true" or "false" like disabled would be wrong because "false" would actually be true.

@titoBouzout
Copy link
Contributor Author

I thought we already did that for psuedo booleans like draggable. For actually boolean properties "true" or "false" like disabled would be wrong because "false" would actually be true.

We do yeah, but I am not sure if consistently, and I would like to make sure we do. I will be looking at this this weekend

@titoBouzout
Copy link
Contributor Author

Updated boolean attributes to accept "true" | boolean, the preferred form should be "true", as it's inlined in the template, etc.

The string "false" should be a type error.

This change only modified real boolean attributes and didn't change enumerated attributes such "yes" | "no", "true" | "false" (such draggable etc)

I'm ok with the pr being reviewed and if I am not missing something to be merged

@titoBouzout
Copy link
Contributor Author

will close
#379
#381

if deprecating classList is also merged it will close
#308
#88
solidjs/solid#2341
solidjs/solid#2050

@webstrand
Copy link

I've checked over my use of classList, the only cases where I use classList={{ [parentClass]: false }} are places where I've screwed up the CSS cascade order and have used classList to disable classes that were preventing me from overriding styles.

@titoBouzout titoBouzout marked this pull request as draft November 13, 2024 22:24
@titoBouzout
Copy link
Contributor Author

titoBouzout commented Nov 13, 2024

I need to make a correction, I will do soon. So I will mark as DRAFT meanwhile

@titoBouzout
Copy link
Contributor Author

OK it's ready again, I double-checked and corrected: experimental, non-standard, deprecated marks

@titoBouzout titoBouzout marked this pull request as ready for review November 14, 2024 16:41
@MrFoxPro
Copy link
Contributor

MrFoxPro commented Nov 17, 2024

  1. I don't care camelCase or flatcase, but do it in single way, so it's easy to fork and change. Or make separate typings for each variant.
  2. I'm not sure it's good thing to deprecate classList.

@webstrand
Copy link

The problem with classList and class is the same as onClick versus onclick, there are two ways to do the same thing. So any component that wraps another has to include logic to handle both cases and resolve conflict when both are specified. I've previously run into issues where one component was written to prefer one form and another the other form.

I do regret that changing class forces the browser (or solid) to re-parse the class string, whereas classList can just be mapped onto Object.entries(classList).map(([className,enabled]) => elem.classList.toggle(className, enabled) though I guess an array string[] would be an even better option for such a mapping.

I believe I read Ryan somewhere saying that adding a typeof props.class === "string" ? setClassName() : reconcileClassList() was prohibitively expensive. That'd introduce complexity of it's own if components wanted to handle both formats, but at least it'd be in a single mutually exclusive prop, and the component can just declare that it only handles strings or arrays and all is well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Property 'xmlns:xlink' does not exist on type 'SvgSVGAttributes<SVGSVGElement>'.
4 participants