Skip to content

Extract rule: template-no-this-in-template-only-components#2575

Merged
NullVoxPopuli merged 3 commits intoember-cli:masterfrom
NullVoxPopuli:nvp/template-lint-extract-rule-template-no-this-in-template-only-components
Mar 22, 2026
Merged

Extract rule: template-no-this-in-template-only-components#2575
NullVoxPopuli merged 3 commits intoember-cli:masterfrom
NullVoxPopuli:nvp/template-lint-extract-rule-template-no-this-in-template-only-components

Conversation

@NullVoxPopuli
Copy link
Copy Markdown
Contributor

Split from #2371.

Copy link
Copy Markdown

@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-no-this-in-template-only-components

Compared against the original ember-template-lint rule at lib/rules/no-this-in-template-only-components.js.

What's done well

  • Core detection logic using GlimmerPathExpression with node.head?.type === 'ThisHead' is clean and correct.
  • The auto-fix from this.foo to @foo is properly implemented.
  • Built-in properties that shouldn't be auto-fixed are handled correctly (reported but not fixed).
  • Tests cover the key cases from the original: basic this.foo, subexpressions, action helpers, and the elementId non-fixable case.
  • Both ember-eslint-parser and ember-eslint-parser/hbs modes are tested.
  • The originallyFrom metadata is a nice touch for traceability.

Differences from the original worth noting

  1. Reduced BUILTIN_PROPERTIES list: The original ember-template-lint rule has a larger set of built-in properties that are excluded from auto-fix: action, actionContext, actionContextObject, concatenatedProperties, element, parentView, target -- in addition to the ones included here (elementId, tagName, ariaRole, class, classNames, classNameBindings, attributeBindings, isVisible). Some of these (like action, element, parentView) could cause incorrect fixes if auto-replaced with @action etc. Consider aligning the list with the original, or documenting why the reduced set is intentional.

  2. No file-path-based detection: The original rule has sophisticated logic to determine whether a template is actually template-only by checking for a co-located component class file on disk (isTemplateOnlyComponent()). The ESLint version skips this entirely and flags all this usage in <template> tags. This is a significant behavioral difference. In gjs/gts, the ESLint rule can rely on AST context (class body vs module level) -- but the rule as written doesn't distinguish between <template> inside a class body vs at module level. This means this.foo inside a class component's <template> would be incorrectly flagged. This seems like a bug -- the rule should check whether the <template> is inside a class body (similar to what PR 2580 does with isInsideClassOrFunction).

  3. validComponentExtensions config option: The original supports a validComponentExtensions config option (defaulting to ['.js', '.ts']). The ESLint version has schema: [] (no options). This is fine for a first pass but worth noting.

  4. Error message: The original includes "...or create a component.js for this component" in the message. The ESLint version omits this, which is reasonable since gjs/gts files are self-contained.

Potential bug

The rule uses GlimmerPathExpression which fires for all <template> tags in the file, including those inside class bodies. A class component like:

class MyComponent extends Component {
  <template>{{this.name}}</template>
}

would be incorrectly flagged. The rule should check that the <template> is at module level (not inside a class or function) before reporting. Consider reusing the isInsideClassOrFunction helper from PR #2580.

Test coverage

  • The valid case {{@foo}} is good. Consider adding a valid test for this inside a class body <template> to verify it's not flagged (once the bug above is fixed).
  • Missing a test for this.foo.bar (nested property access) -- the fix should produce @foo.bar.

🤖 Automated review comparing with ember-template-lint source

NullVoxPopuli and others added 2 commits March 21, 2026 21:49
The rule was incorrectly flagging `this` usage in ALL `<template>` tags,
including those inside class bodies (class components). It should only
flag `this` in template-only components (standalone `<template>` tags).

Also adds missing BUILTIN_PROPERTIES from the original ember-template-lint
rule: action, element, parentView, attrs, isDestroying, isDestroyed.

Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
@NullVoxPopuli NullVoxPopuli force-pushed the nvp/template-lint-extract-rule-template-no-this-in-template-only-components branch from 41758b1 to c23fa18 Compare March 22, 2026 01:49
@NullVoxPopuli NullVoxPopuli enabled auto-merge March 22, 2026 02:05
@NullVoxPopuli NullVoxPopuli merged commit b430b92 into ember-cli:master Mar 22, 2026
9 checks passed
@NullVoxPopuli NullVoxPopuli deleted the nvp/template-lint-extract-rule-template-no-this-in-template-only-components branch March 22, 2026 02:10
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