Skip to content

Conversation

@yeonjuan
Copy link
Owner

@yeonjuan yeonjuan commented Aug 7, 2025

Checklist

Description

@yeonjuan yeonjuan changed the title No ineffective attrs feat: add new rule no-ineffective-attrs Aug 7, 2025
@yeonjuan yeonjuan requested a review from Copilot August 9, 2025 09:52
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR introduces a new ESLint rule called no-ineffective-attrs that detects HTML attributes that have no effect in their current context. The rule helps identify potential errors or unnecessary code by flagging attributes like multiple on text inputs, defer on inline scripts, or download without href.

  • Adds a new ESLint rule implementation with comprehensive validation logic
  • Includes extensive test coverage for both regular HTML and template literals
  • Provides documentation with clear examples of correct and incorrect usage

Reviewed Changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
packages/eslint-plugin/lib/rules/no-ineffective-attrs.js Core rule implementation that checks for ineffective HTML attributes across different element types
packages/eslint-plugin/tests/rules/no-ineffective-attrs.test.js Comprehensive test suite covering valid and invalid attribute combinations
packages/eslint-plugin/lib/rules/utils/node.js Adds utility function hasAttr for checking attribute presence
packages/eslint-plugin/lib/rules/index.js Registers the new rule in the rules index
docs/rules/no-ineffective-attrs.md User documentation with usage examples and rule explanation
.cspell.json Adds spell check exceptions for HTML attribute values

{
attr: "multiple",
when: (node) => {
const type = getAttrValue(node, "type") || "text";
Copy link

Copilot AI Aug 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The magic string "text" is used as a default input type. Consider extracting this as a named constant to improve maintainability and reduce duplication (it appears on lines 32, 49, and 58).

Suggested change
const type = getAttrValue(node, "type") || "text";
const type = getAttrValue(node, "type") || DEFAULT_INPUT_TYPE;

Copilot uses AI. Check for mistakes.
Comment on lines +127 to +169
const tagCheckers = checkersByTag[node.name];
if (!tagCheckers) return;

for (const check of tagCheckers) {
for (const attr of node.attributes) {
if (attr.type !== "Attribute") continue;
if (attr.key.value !== check.attr) continue;

if (check.when(node)) {
context.report({
loc: attr.loc,
messageId: "ineffective",
data: {
message: check.message,
},
});
}
}
}
},
/**
* @param {ScriptTag} node
*/
ScriptTag(node) {
const scriptCheckers = checkersByTag.script;
if (!scriptCheckers) return;

for (const check of scriptCheckers) {
for (const attr of node.attributes) {
if (attr.type !== "Attribute") continue;
if (attr.key.value !== check.attr) continue;

if (check.when(node)) {
context.report({
loc: attr.loc,
messageId: "ineffective",
data: {
message: check.message,
},
});
}
}
}
Copy link

Copilot AI Aug 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The logic in the ScriptTag visitor is nearly identical to the Tag visitor (lines 126-146). Consider extracting the common attribute checking logic into a shared helper function to reduce code duplication.

Suggested change
const tagCheckers = checkersByTag[node.name];
if (!tagCheckers) return;
for (const check of tagCheckers) {
for (const attr of node.attributes) {
if (attr.type !== "Attribute") continue;
if (attr.key.value !== check.attr) continue;
if (check.when(node)) {
context.report({
loc: attr.loc,
messageId: "ineffective",
data: {
message: check.message,
},
});
}
}
}
},
/**
* @param {ScriptTag} node
*/
ScriptTag(node) {
const scriptCheckers = checkersByTag.script;
if (!scriptCheckers) return;
for (const check of scriptCheckers) {
for (const attr of node.attributes) {
if (attr.type !== "Attribute") continue;
if (attr.key.value !== check.attr) continue;
if (check.when(node)) {
context.report({
loc: attr.loc,
messageId: "ineffective",
data: {
message: check.message,
},
});
}
}
}
checkIneffectiveAttrs(node, checkersByTag[node.name], context);
},
/**
* @param {ScriptTag} node
*/
ScriptTag(node) {
checkIneffectiveAttrs(node, checkersByTag.script, context);

Copilot uses AI. Check for mistakes.
Comment on lines +33 to +42
return [
"text",
"password",
"radio",
"checkbox",
"image",
"hidden",
"reset",
"button",
].includes(type);
Copy link

Copilot AI Aug 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The array of input types that don't support 'multiple' should be extracted as a constant. This improves maintainability and makes it easier to update the list if HTML specifications change.

Suggested change
return [
"text",
"password",
"radio",
"checkbox",
"image",
"hidden",
"reset",
"button",
].includes(type);
return INPUT_TYPES_WITHOUT_MULTIPLE.includes(type);

Copilot uses AI. Check for mistakes.
attr: "readonly",
when: (node) => {
const type = getAttrValue(node, "type") || "text";
return ["checkbox", "radio", "file", "range", "color"].includes(type);
Copy link

Copilot AI Aug 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The array of input types that don't support 'readonly' should be extracted as a constant for better maintainability and consistency with other type checking logic.

Suggested change
return ["checkbox", "radio", "file", "range", "color"].includes(type);
return INPUT_TYPES_WITHOUT_READONLY.includes(type);

Copilot uses AI. Check for mistakes.
@yeonjuan yeonjuan marked this pull request as ready for review August 9, 2025 10:02
@yeonjuan yeonjuan merged commit e05c325 into main Aug 9, 2025
5 checks passed
@yeonjuan yeonjuan deleted the no-ineffective-attrs branch August 9, 2025 10:04
@codecov
Copy link

codecov bot commented Aug 9, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 98.56%. Comparing base (9d00cec) to head (286ba84).
⚠️ Report is 2 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #399      +/-   ##
==========================================
+ Coverage   98.53%   98.56%   +0.02%     
==========================================
  Files          82       83       +1     
  Lines        2658     2712      +54     
  Branches      736      752      +16     
==========================================
+ Hits         2619     2673      +54     
  Misses         39       39              
Flag Coverage Δ
unittest 98.56% <100.00%> (+0.02%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
packages/eslint-plugin/lib/rules/index.js 100.00% <100.00%> (ø)
...es/eslint-plugin/lib/rules/no-ineffective-attrs.js 100.00% <100.00%> (ø)
packages/eslint-plugin/lib/rules/utils/node.js 95.77% <100.00%> (+0.18%) ⬆️

... and 1 file with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[FEATURE] Add no-ineffective-attrs rule

2 participants