-
Notifications
You must be signed in to change notification settings - Fork 129
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 for attributes and properties #380
base: main
Are you sure you want to change the base?
Conversation
…so we dont have to check why is not there, we will know it has been deprecated)
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 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 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.. |
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 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 Additionally, I am thinking that a 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 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 |
Not sure if they're desired but I noticed the lack of the non-standard attribute |
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. |
I don't really care whether 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
|
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.
I thought we already did that for psuedo booleans like |
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 |
…e"`. The string `"false"` should be a type error
Updated The string This change only modified real I'm ok with the pr being reviewed and if I am not missing something to be merged |
if deprecating classList is also merged it will close |
I've checked over my use of |
I need to make a correction, I will do soon. So I will mark as DRAFT meanwhile |
OK it's ready again, I double-checked and corrected: experimental, non-standard, deprecated marks |
|
The problem with I do regret that changing I believe I read Ryan somewhere saying that adding a |
Found some missing attributes, on I'm not sure if I missed something, weren't the camelCase or the flatcase general attributes being marked as deprecated in this PR? For instance |
Thanks, just added them, and removed
Yes, and I agree it would be nice to do the same for event listeners, but I am not sure ... people seem to be too used to the The main point of this PR is to remove a slice of the confusion generated by attributes vs properties. |
Couple of changes and updates to typings:
@deprecated
any attribute usingcamelCase
in case-insensitive areas, such HTML. (made sure there's a lowercase version for the attributes changed)HTMLQuoteElement
instead ofHTMLElement
MathML
typings (this mostly comes from Preact)WebView
deprecateclassList
with messageUse "clsx" instead. https://www.npmjs.com/package/clsx
@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 deprecatedclassList
, shall we?