Skip to content

Extract rule: template-require-button-type#2600

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

Extract rule: template-require-button-type#2600
NullVoxPopuli merged 2 commits intoember-cli:masterfrom
NullVoxPopuli:nvp/template-lint-extract-rule-template-require-button-type

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-button-type (PR #2600)

Comparison with ember-template-lint require-button-type.js

General correctness:

  1. Core logic matches: The rule checks <button> elements for a valid type attribute with values button, submit, or reset. Missing type or invalid type is reported. This matches the original.

  2. Form-aware fix: Both the original and the PR check for a parent <form> element to decide the default fix value (submit inside form, button otherwise). The original uses hasParentTag(path, 'form') from a shared helper. The PR implements its own hasParentForm(node) that walks node.parent. This is functionally equivalent.

  3. Fix for invalid type — behavior difference: The original's fix for invalid type values always uses b.text('button') (hardcoded to "button"). The PR's fix uses hasParentForm(node) ? 'submit' : 'button', so it's context-aware even for the invalid-type case. This is actually an improvement over the original — if a button inside a form has type="foo", it makes more sense to fix it to type="submit" than type="button".

  4. Dynamic type values: The original returns early for non-TextNode type values (if (value.type !== 'TextNode') return). The PR has a comment "Dynamic type values cannot be validated at lint time" and similarly doesn't report them. Correct match.

  5. Self-closing button fix: The PR has special handling for <button/><button type="button" />. The original uses b.attr(...) to push to attributes (AST builder approach). The PR's regex-based approach (/^<button\s*\/>/.test(text)) is a bit fragile — it wouldn't handle <button class="x"/> (button with other attributes but no type). However, since this code path is only reached when typeAttr is missing, a self-closing button with other attributes would still hit the openTag regex path, which should work.

  6. Tests are comprehensive: Both gjs and hbs modes, valid/invalid cases with fixes, form context, edge cases like type=42, and even a JS comment containing <button> to test parser robustness.

Scope analysis (gjs/gts):

This rule checks HTML <button> elements — pure structural/tag-name matching. No helper/component name resolution. No scope analysis needed.

Overall: Clean, well-tested migration with a nice improvement on the fix logic for invalid types inside forms.

🤖 Automated review comparing with ember-template-lint source

@NullVoxPopuli NullVoxPopuli force-pushed the nvp/template-lint-extract-rule-template-require-button-type branch from a7f216d to cf53182 Compare March 20, 2026 19:00
@NullVoxPopuli NullVoxPopuli merged commit 5d0d156 into ember-cli:master Mar 20, 2026
11 checks passed
@NullVoxPopuli NullVoxPopuli deleted the nvp/template-lint-extract-rule-template-require-button-type branch March 20, 2026 19:21
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