Extract rule: template-require-has-block-helper#2604
Extract rule: template-require-has-block-helper#2604NullVoxPopuli wants to merge 1 commit intoember-cli:masterfrom
Conversation
NullVoxPopuli-ai-agent
left a comment
There was a problem hiding this comment.
Review: template-require-has-block-helper
General Correctness
1. Error message differs from original and loses specificity. The original produces context-specific messages like `hasBlock` is deprecated. Use the `has-block` helper instead. and `hasBlockParams` is deprecated. Use the `has-block-params` helper instead. The PR uses a single generic message: "Use (has-block) helper instead of hasBlock property." for all cases, including hasBlockParams. This is misleading -- when the user writes hasBlockParams, the message should suggest has-block-params, not has-block. Consider using two separate messages or a parameterized message with data.
2. Missing detection for this.hasBlock and this.hasBlockParams. Wait, actually the PR does check for this.hasBlock and this.hasBlockParams -- good. But the original does NOT check for this.hasBlock or this.hasBlockParams -- it only matches raw hasBlock and hasBlockParams PathExpressions. The this.hasBlock form is a different thing (it's a property access on this). Adding this.hasBlock and this.hasBlockParams may cause false positives if a component legitimately has a hasBlock property (distinct from the template keyword). However, since this rule is templateMode: 'loose' (HBS only), the this.hasBlock case could be argued as a valid lint target in classic templates. Still, this is a behavioral divergence from the original.
3. The original supports auto-fix mode (this.mode === 'fix') with specific transformations. The PR sets fixable: null, which is fine for a first pass -- the fix logic is complex and can be added later.
4. templateMode: 'loose' is correctly set -- this rule is only meaningful in classic .hbs files where hasBlock/hasBlockParams are implicit globals. In gjs/gts strict mode, these don't exist. Good.
5. Tests are comprehensive with good coverage of both gjs and hbs modes, and many edge cases from the original.
Scope Analysis (gjs/gts)
The rule is marked templateMode: 'loose', meaning it only applies to .hbs files. In .hbs files, there's no JS scope to analyze. No scope analysis needed.
However, I notice the tests DO include gjs <template> tests that trigger the rule. If templateMode: 'loose' truly restricts the rule to HBS only, those gjs tests should not produce errors. If the gjs tests pass (meaning the rule DOES run in gjs), then templateMode: 'loose' may not be enforced by the framework, and this could produce false positives in gjs/gts files where hasBlock could be a JS variable in scope. Worth verifying.
Summary
The main issues are: (1) the generic error message that doesn't distinguish hasBlock vs hasBlockParams, and (2) the addition of this.hasBlock/this.hasBlockParams detection that doesn't exist in the original. Also verify that templateMode: 'loose' actually prevents the rule from running in gjs mode, given the gjs test cases.
🤖 Automated review comparing with ember-template-lint source
Split from #2371.