Skip to content

Extract rule: template-require-mandatory-role-attributes#2609

Merged
NullVoxPopuli merged 2 commits intoember-cli:masterfrom
NullVoxPopuli:nvp/template-lint-extract-rule-template-require-mandatory-role-attributes
Mar 20, 2026
Merged

Extract rule: template-require-mandatory-role-attributes#2609
NullVoxPopuli merged 2 commits intoember-cli:masterfrom
NullVoxPopuli:nvp/template-lint-extract-rule-template-require-mandatory-role-attributes

Conversation

@NullVoxPopuli
Copy link
Contributor

Split from #2371.

Copy link

@NullVoxPopuli-ai-agent NullVoxPopuli-ai-agent left a comment

Choose a reason for hiding this comment

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

Review: template-require-mandatory-role-attributes

General Correctness

1. Hardcoded role-to-attribute mapping vs. original's aria-query dependency. The original uses import { roles } from 'aria-query' to dynamically look up required attributes from the ARIA spec. The PR hardcodes a MANDATORY_ROLE_ATTRIBUTES map. This means:

  • If the ARIA spec is updated (new roles or changed requirements), the original auto-updates via the aria-query package, but the PR's hardcoded map becomes stale.
  • The PR may be missing roles that aria-query knows about. For example, the original would catch any role defined in aria-query with required props, while the PR only handles 10 specific roles.
  • Recommendation: Consider importing from aria-query to stay in sync with the spec, or at minimum document that this is a subset.

2. The heading role requires aria-level in the PR but this is not universally required. Looking at the ARIA spec and aria-query, heading does have aria-level as a required attribute. The PR includes this; good.

3. Error message format differs from original. The original produces a single message for all missing attributes: "The attributes aria-valuenow, aria-valuemin, aria-valuemax are required by the role slider". The PR reports ONE error per missing attribute: "Role "slider" requires ARIA attribute "aria-valuenow" to be present." This is actually better for ESLint since each error can be individually located, but it means a role="slider" without any aria attrs produces 3 separate errors instead of 1. The tests account for this (expecting 3 errors for slider).

4. The GlimmerMustacheStatement handler correctly handles curly component invocations with role hash pairs. The original also handles this. Good parity.

5. The getRoleValue function only handles GlimmerTextNode (static text values), skipping dynamic roles. The original also skips non-TextNode roles. Consistent.

6. Missing: the original checks roleDefinition === undefined to skip unknown roles. The PR achieves this via !MANDATORY_ROLE_ATTRIBUTES[role], which returns undefined for unknown roles and correctly skips. Equivalent.

Scope Analysis (gjs/gts)

The GlimmerElementNode handler checks node.attributes for role -- this is HTML attribute inspection on elements. No scope analysis needed for this path.

The GlimmerMustacheStatement handler checks node.path?.original but only to get the component name for context, and the actual logic is based on hash.pairs (the role key-value pair). The rule doesn't match on specific helper/component names -- it checks any mustache statement for a role hash pair. No scope analysis needed since the rule is checking attribute semantics, not matching specific component/helper names.

Summary

The main concern is the hardcoded role-to-attribute mapping vs. the original's dynamic aria-query lookup. This risks going stale as the ARIA spec evolves. The per-attribute error reporting (vs. original's per-role single error) is a reasonable design choice for ESLint. Otherwise, a solid migration.

🤖 Automated review comparing with ember-template-lint source

@NullVoxPopuli NullVoxPopuli force-pushed the nvp/template-lint-extract-rule-template-require-mandatory-role-attributes branch from f92ac4e to 727df22 Compare March 20, 2026 03:19
@NullVoxPopuli NullVoxPopuli merged commit 3f82e2a into ember-cli:master Mar 20, 2026
9 checks passed
@NullVoxPopuli NullVoxPopuli deleted the nvp/template-lint-extract-rule-template-require-mandatory-role-attributes branch March 20, 2026 03:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants