Skip to content

Extract rule: template-require-has-block-helper#2604

Open
NullVoxPopuli wants to merge 1 commit intoember-cli:masterfrom
NullVoxPopuli:nvp/template-lint-extract-rule-template-require-has-block-helper
Open

Extract rule: template-require-has-block-helper#2604
NullVoxPopuli wants to merge 1 commit intoember-cli:masterfrom
NullVoxPopuli:nvp/template-lint-extract-rule-template-require-has-block-helper

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-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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants