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

New Rule Proposal: no-unnecessary-html-attributes #2866

Open
karlhorky opened this issue Dec 3, 2020 · 17 comments
Open

New Rule Proposal: no-unnecessary-html-attributes #2866

karlhorky opened this issue Dec 3, 2020 · 17 comments

Comments

@karlhorky
Copy link
Contributor

karlhorky commented Dec 3, 2020

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 value type="text" on input elements, but due to maintainer feedback, it was modified to include default attributes on more elements.

Example of incorrect code:

<input type="text" />

<button type="submit">submit</button>

<ol type="1">
  <li type="1">first</li>
</ol>

<style type="text/css">{`a { color: red; }`}</style>

<link type="text/css" rel="stylesheet" href="https://example.com/main.css" />

<form method="GET"></form>
<form enctype="application/x-www-form-urlencoded"></form>
<form target="_self"></form>

<img loading="eager" src="https://example.com/dog.jpg" alt="Doge" />

<iframe loading="eager" src="https://example.com"></iframe>

<textarea wrap="soft"></textarea>

Examples of correct code:

<input />
<input type="number" />

<button>submit</button>
<button type="button">submit</button>

<ol>
  <li>first</li>
</ol>

<style>{`a { color: red; }`}</style>

<link rel="stylesheet" href="https://example.com/main.css" />

<form></form>

<img src="https://example.com/dog.jpg" alt="Doge" />

<iframe src="https://example.com"></iframe>

<textarea></textarea>
@ljharb
Copy link
Member

ljharb commented Dec 3, 2020

It's far, far better to always require explicitness, even when things have defaults. I'd rather see a rule that forbids <input /> than one that requires people to keep an extra thing in their heads (that the default type is "text").

@karlhorky
Copy link
Contributor Author

karlhorky commented Dec 3, 2020

It's far, far better to always require explicitness, even when things have defaults

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 inputs, this will reduce the amount of code that needs to be written / reviewed.

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: never, one for your proposal, eg. always) There's a lot of prior art of ESLint rules that offer options for both opinions.

@ljharb
Copy link
Member

ljharb commented Dec 3, 2020

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.

@karlhorky
Copy link
Contributor Author

Are there other HTML elements that this would apply to, where the default can be omitted and also is unsurprising?

Looks like <ol> and the <li> elements contained within have type attributes:

  • a for lowercase letters
  • A for uppercase letters
  • i for lowercase Roman numerals
  • I for uppercase Roman numerals
  • 1 for numbers (default)

<ol> on MDN

The <ul> element also has a type attribute, but this attribute is deprecated.

Also (but maybe less interestingly):

  • <style> - default: type="text/css"
  • <script> - default: type="text/javascript"

@ljharb
Copy link
Member

ljharb commented Dec 4, 2020

huh, i've never heard of those being used - only the CSS.

In React, <script> doesn't really make sense to address. So that would basically leave us with button, input, and style - not all that many :-/

@karlhorky karlhorky changed the title New Rule Proposal: no-input-type-text New Rule Proposal: no-unnecessary-type-props Dec 5, 2020
@karlhorky
Copy link
Contributor Author

karlhorky commented Dec 5, 2020

Hmm... and what's your opinion on other default props / HTML attributes? Things that can be omitted and the functionality is the same?

Eg:

  • <form method="GET">
  • <form enctype="application/x-www-form-urlencoded">

Can also search for more if this would be a way forward.

@ljharb
Copy link
Member

ljharb commented Dec 5, 2020

I think the longer a list we build up, the more convincing a rule for it becomes.

@karlhorky
Copy link
Contributor Author

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.

@karlhorky karlhorky changed the title New Rule Proposal: no-unnecessary-type-props New Rule Proposal: no-unnecessary-html-attributes Dec 5, 2020
@karlhorky
Copy link
Contributor Author

Ok, so I've updated the list above with more examples.

Talking with @wooorm, it seems like there are some interesting resources in the rehype-minify-enumerated-attribute package which could help with finding the attributes with default values.

I've asked if publishing a separate package with this data would be possible at rehypejs/rehype-minify#37

@ljharb
Copy link
Member

ljharb commented Dec 6, 2020

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.

@wooorm
Copy link

wooorm commented Dec 8, 2020

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.

@ljharb
Copy link
Member

ljharb commented Dec 8, 2020

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)

@wooorm
Copy link

wooorm commented Dec 10, 2020

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).
It’s relevant here because the HTML minifier in question uses hast.

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

@ljharb
Copy link
Member

ljharb commented Dec 29, 2020

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?

@karlhorky
Copy link
Contributor Author

Some more discussion about publishing the new package over here: rehypejs/rehype-minify#37

@karlhorky
Copy link
Contributor Author

karlhorky commented Sep 10, 2021

Created a rudimentary version to prevent <input type="text" /> and <button type="submit"></button>:

Code: https://github.com/upleveled/eslint-plugin-upleveled/blob/main/rules/no-unnecessary-html-attributes.ts
Docs: https://github.com/upleveled/eslint-plugin-upleveled/blob/main/docs/rules/no-unnecessary-html-attributes.md

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.

@ljharb
Copy link
Member

ljharb commented Sep 10, 2021

@karlhorky i'm still hopeful this can rule can be upstreamed here, once there's a core package shared by us and hast.

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

No branches or pull requests

3 participants