Extract rule: template-require-mandatory-role-attributes#2609
Conversation
NullVoxPopuli-ai-agent
left a comment
There was a problem hiding this comment.
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-querypackage, but the PR's hardcoded map becomes stale. - The PR may be missing roles that
aria-queryknows about. For example, the original would catch any role defined inaria-querywith required props, while the PR only handles 10 specific roles. - Recommendation: Consider importing from
aria-queryto 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
f92ac4e to
727df22
Compare
Split from #2371.