-
-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
New Rule Proposal: no-unnecessary-html-attributes
#2866
Comments
It's far, far better to always require explicitness, even when things have defaults. I'd rather see a rule that forbids |
I understand the mindset, and in some cases, I agree. But under some circumstances, it's nice to not have to write the verbose thing. And if you have a lot of code with Also, and maybe more importantly, this is an opinion - other individuals and teams will have different ones :) So I guess my question would be: why not both? (one option value for what I'm proposing eg: |
I don't think that just "input" is sufficient to warrant an entire rule. There's also already a rule, react/button-has-type, for button elements, but that's because the default is unintuitive. Are there other HTML elements that this would apply to, where the default can be omitted and also is unsurprising? A single rule that encompasses a bunch of HTML elements in this way, autofixable, with both options, seems like it might be worth the complexity. |
Looks like
The Also (but maybe less interestingly):
|
huh, i've never heard of those being used - only the CSS. In React, |
no-input-type-text
no-unnecessary-type-props
Hmm... and what's your opinion on other default props / HTML attributes? Things that can be omitted and the functionality is the same? Eg:
Can also search for more if this would be a way forward. |
I think the longer a list we build up, the more convincing a rule for it becomes. |
Ok, let me do a bit of research, and I will come back with a longer list. I'll change the issue title and description in the meantime. |
no-unnecessary-type-props
no-unnecessary-html-attributes
Ok, so I've updated the list above with more examples. Talking with @wooorm, it seems like there are some interesting resources in the I've asked if publishing a separate package with this data would be possible at rehypejs/rehype-minify#37 |
I have https://npmjs.com/html-element-map, which won’t help here but is a similar concept re extracted data as a package, so that would likely work well. |
Yeah I’m fine publishing it separately! The Q then is: what format do you need? Does the current schema make sense? Another thing is that the schema currently uses hast casing, which is in rare cases different from react casing (although the translation is relatively simple), and both are different from HTML. |
Note that publishing it separately is most compelling with other projects can start using it - that way it ensures it will stay correct :-) I have no idea what "hast" is, but it seems reasonable for the package to provide html casing, and for consumers to adapt it as needed (or, for another package to wrap it and provide the react version, eg) |
hast is an html ast. Like estree but for HTML. Sometimes but not always used with mdast (markdown). Transformation between react / hast / html / dom is all not very complex, but does need a bit of a look up table. As we’re currently interested in react / hast (almost identical, close to DOM props), using that sounds like the simplest path forward to me? Anyway, I’m not blocking this, just preferences |
Ideally I'd like this plugin to use something that's very simple and barebones - just DOM to react jsx - that shares a common core with what the minifier uses, rather than using something multi-purpose that will have more version churn. Is that a possibility? |
Some more discussion about publishing the new package over here: rehypejs/rehype-minify#37 |
Created a rudimentary version to prevent Code: https://github.com/upleveled/eslint-plugin-upleveled/blob/main/rules/no-unnecessary-html-attributes.ts Will be published at the following URL, once npm stops having publishing problems today: https://www.npmjs.com/package/@upleveled/eslint-plugin-upleveled First plugin rule I've written, wild ride working with the AST! 🙌 And also finding the types for TypeScript. |
@karlhorky i'm still hopeful this can rule can be upstreamed here, once there's a core package shared by us and hast. |
Hi there 👋
Thanks for the continued work on this great plugin!
I had an idea for a rule today, and wanted to test the waters here on a
no-unnecessary-html-attributes
rule. It originally was inspired due to the default valuetype="text"
oninput
elements, but due to maintainer feedback, it was modified to include default attributes on more elements.Example of incorrect code:
Examples of correct code:
The text was updated successfully, but these errors were encountered: