eslint-plugin-react icon indicating copy to clipboard operation
eslint-plugin-react copied to clipboard

[Feat]: require-dom-props for enforcing properties on DOM Elements

Open intellix opened this issue 2 years ago • 4 comments

Is there an existing issue for this?

  • [X] I have searched the existing issues and my issue is unique
  • [X] My issue appears in the command-line and not only in the text editor

Description Overview

So you've optimised your bundles, your images and improved your Lighthouse score and someone adds an <iframe> element to the bottom of the homepage containing a grid of youtube video previews. Lighthouse tells you that you didn't need that extra 8mb on your homepage.

Not everyone knows about the <iframe loading="lazy"> property to fix this and it was an oversight not to include it. In order to fix this, it would be great to have a linting rule that enforced you to explicitly provide properties to certain DOM elements, like iframe. That way when it was added, in order for the regression, a developer would have had to explicitly state loading="eager" (the default).

I can see that there are rules for forbidding certain properties on DOM/Components but I don't see a rule for requiring properties to exist on certain elements and components.

If a more generic rule was added that supported this, you could essentially deprecate this property as it would be achieveable via the new rule: https://github.com/jsx-eslint/eslint-plugin-react/blob/master/docs/rules/iframe-missing-sandbox.md

Expected Behavior

I should get a linting warning when someone adds an iframe and doesn't provide the loading property.

eslint-plugin-react version

N/A

eslint version

N/A

node version

N/A

intellix avatar Oct 14 '23 13:10 intellix

We do have https://github.com/jsx-eslint/eslint-plugin-react/blob/master/docs/rules/button-has-type.md for that specific issue, but adding one for iframe loading seems like it wouldn't always be a good idea.

Are there any other examples where someone would want to enforce an explicit attribute where the default behavior is likely to cause a bug, such that it would warrant a generic rule?

ljharb avatar Oct 14 '23 16:10 ljharb

MDN suggests that the default value is "eager" and forcing the developer to specify if they prefer eager or lazy gives them to the opportunity to think about if they really need it loading straight away: https://developer.mozilla.org/en-US/docs/Web/HTML/Element/iframe#loading

<iframe src="heavy-page.com"></iframe>

vs:

<iframe src="heavy-page.com" loading="eager"></iframe>

Why wouldn't it be a good idea? Today, when a frontender adds an iframe, the fact that it may load an additional 5mb eagerly doesn't even enter their mind. Now that they've explicitly said it needs to be provided, it gives them the opportunity to think: "hang on a minute... this can probably be lazy because it's below the page fold"

For those 3 use cases it could be merged into a singular rule, and you can extend it further, like enforcing the recommendations by Lighthouse for IMG tags where you want to reduce CLS (Cumulative Layout Shift). Forgive me if the syntax sucks but just trying to show what I would personally configure such a rule with:

{
  "requireDomProps": {
     iframe: ["loading", "sandbox"],
     button: ["type"],
     img: ["width", "height", "decoding", "loading", "alt"],
     video: ["width", "height", "preload"]
   }
}

intellix avatar Oct 14 '23 17:10 intellix

I've been a frontender a long time, and I never even know that iframes could be lazy before now - for decades, they've always been eager and that's what everyone's expectation is and should be.

img doesn't need both a width and height necessarily; just one of them is fine. alt is for a11y (and is handled by eslint-plugin-jsx-a11y), and decoding and loading i've never heard of.

ljharb avatar Oct 14 '23 21:10 ljharb

Yeah an image doesn't need a height defining but if you want a better user experience, lighthouse score and better SEO (from that score) then if you're aware of the dimensions up front it prevents CLS, content shifting around as images are discovered and loading in. I'd like to enforce that in my project for those tags.

It's just that it would be good to enforce these things via linting and if they're configurable it's easy to add/remove

intellix avatar Oct 14 '23 22:10 intellix