Extract rule: template-no-unnecessary-component-helper#2584
Conversation
NullVoxPopuli-ai-agent
left a comment
There was a problem hiding this comment.
Review: template-no-unnecessary-component-helper (comparing with ember-template-lint no-unnecessary-component-helper)
Coverage
Good migration that covers the key behavior of the original rule.
What's done well
- Correct detection logic:
isComponentWithStringLiteralchecks the same conditions as the original --path.original === 'component', first param is aStringLiteral, and the string doesn't contain@(addon-scoped names). - Attribute-safe namespace tracking: The
inAttributecounter correctly mirrors the original'sinSafeNamespacepattern (usingAttrNodeenter/exit), ensuring that{{component "name"}}inside attribute values is not flagged. This is a key behavior of the original. - Auto-fix for MustacheStatement and SubExpression: Fixes correctly reconstruct the invocation by stripping the
componenthelper and using the component name directly. - No fix for BlockStatement: Correctly does not attempt to fix block invocations (matching the original's more complex block fix that may not translate cleanly).
- Comprehensive tests: Good coverage of valid/invalid cases in both gjs and hbs modes.
Items needing attention
-
SubExpression handling is an addition: The original only handles
MustacheStatementandBlockStatementvisitors. This PR addsGlimmerSubExpressionhandling. While this is arguably a useful extension, it goes beyond the original rule's scope. The original explicitly only visitsAttrNode,BlockStatement, andMustacheStatement. In practice, a sub-expression like(component "foo")in a non-attribute context is unusual, so this may be fine, but it's worth flagging as a divergence. -
(component "my-component")in HBS valid tests: The HBS valid test includes'(component "my-component")'as valid. This is correct because in HBS mode, a bare sub-expression at the top level is unusual, but more importantly, the rule correctly exempts sub-expressions in attribute contexts. However, the gjs tests don't test this case explicitly. -
Error message difference: Original says
"Invoke component directly instead of using \component` helper", while the PR says"Unnecessary use of (component) helper. Use the component name directly."`. The PR message is arguably clearer and more actionable.
Verdict
Solid migration. The SubExpression addition is a minor divergence worth documenting but is a reasonable enhancement. Test coverage is good.
🤖 Automated review comparing with ember-template-lint source
92470cb to
d25c2d3
Compare
Split from #2371.