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

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

Open
2 tasks done
intellix opened this issue Oct 14, 2023 · 4 comments
Open
2 tasks done

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

intellix opened this issue Oct 14, 2023 · 4 comments

Comments

@intellix
Copy link

Is there an existing issue for this?

  • I have searched the existing issues and my issue is unique
  • 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 intellix added the bug label Oct 14, 2023
@ljharb
Copy link
Member

ljharb commented Oct 14, 2023

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?

@intellix
Copy link
Author

intellix commented Oct 14, 2023

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"]
   }
}

@ljharb
Copy link
Member

ljharb commented Oct 14, 2023

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.

@intellix
Copy link
Author

intellix commented Oct 14, 2023

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

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

No branches or pull requests

2 participants