Skip to content

Commit 017d63c

Browse files
NullVoxPopuliNullVoxPopuli
authored
Merge pull request #2645 from johanrd/post-merge-review/no-this-in-template-only-components
Post-merge-review: Fix `template-no-this-in-template-only-components`: detect `.hbs` files with backing class on disk
2 parents 3012001 + a950a9f commit 017d63c

7 files changed

Lines changed: 176 additions & 15 deletions

File tree

‎README.md‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -261,7 +261,7 @@ rules in templates can be disabled with eslint directives with mustache or html
261261
| [template-no-redundant-fn](docs/rules/template-no-redundant-fn.md) | disallow unnecessary usage of (fn) helper | | 🔧 | |
262262
| [template-no-restricted-invocations](docs/rules/template-no-restricted-invocations.md) | disallow certain components, helpers or modifiers from being used | | | |
263263
| [template-no-splattributes-with-class](docs/rules/template-no-splattributes-with-class.md) | disallow splattributes with class attribute | | | |
264-
| [template-no-this-in-template-only-components](docs/rules/template-no-this-in-template-only-components.md) | disallow this in template-only components (gjs/gts) | | 🔧 | |
264+
| [template-no-this-in-template-only-components](docs/rules/template-no-this-in-template-only-components.md) | disallow this in template-only components | | 🔧 | |
265265
| [template-no-trailing-spaces](docs/rules/template-no-trailing-spaces.md) | disallow trailing whitespace at the end of lines in templates | | 🔧 | |
266266
| [template-no-unavailable-this](docs/rules/template-no-unavailable-this.md) | disallow `this` in templates that are not inside a class or function | | | |
267267
| [template-no-unnecessary-component-helper](docs/rules/template-no-unnecessary-component-helper.md) | disallow unnecessary component helper | | 🔧 | |

‎lib/rules/template-no-this-in-template-only-components.js‎

Lines changed: 84 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,68 @@
1+
const { existsSync } = require('node:fs');
2+
const path = require('node:path');
3+
4+
const VALID_COMPONENT_EXTENSIONS = ['.js', '.ts'];
5+
const GLIMMER_SCRIPT_EXTENSIONS = ['.gjs', '.gts'];
6+
7+
function componentClassExists(pathWithoutExtension) {
8+
return VALID_COMPONENT_EXTENSIONS.some((ext) => existsSync(pathWithoutExtension + ext));
9+
}
10+
11+
// Port of ember-template-lint's no-this-in-template-only-components#isTemplateOnlyComponent.
12+
// Returns true when the template has no backing component class file on disk.
13+
function isTemplateOnlyComponent(templateFilePath) {
14+
// .gjs/.gts files always have JS context, so they cannot be template-only.
15+
if (GLIMMER_SCRIPT_EXTENSIONS.some((ext) => templateFilePath.endsWith(ext))) {
16+
return false;
17+
}
18+
19+
const allSegments = path.normalize(templateFilePath).split(path.sep);
20+
21+
const appIndex = allSegments.findIndex((seg) => seg === 'app' || seg === 'addon');
22+
if (appIndex === -1) {
23+
// No app/addon directory found — cannot determine layout, don't flag.
24+
return false;
25+
}
26+
// Preserve everything before `app`/`addon` as a prefix so we can rebuild
27+
// absolute class file paths (upstream uses relative paths, which only
28+
// works when the cwd happens to contain `app/`).
29+
const prefix = allSegments.slice(0, appIndex).join(path.sep);
30+
const segments = allSegments.slice(appIndex);
31+
32+
if (segments[1] === 'templates') {
33+
if (segments[2] === 'components') {
34+
// Classic structure: app/templates/components/foo.hbs => app/components/foo.{js,ts}
35+
const moduleName = path.basename(templateFilePath, '.hbs');
36+
const classFilePath = path.join(
37+
prefix,
38+
segments[0],
39+
'components',
40+
...segments.slice(3, -1),
41+
moduleName
42+
);
43+
return !componentClassExists(classFilePath);
44+
}
45+
// Route template — always has a backing controller/route context.
46+
return false;
47+
}
48+
49+
if (segments[1] === 'components') {
50+
// Co-located structure: app/components/foo.hbs => app/components/foo.{js,ts}
51+
const moduleName = path.basename(templateFilePath, '.hbs');
52+
const classFilePath = path.join(path.dirname(templateFilePath), moduleName);
53+
return !componentClassExists(classFilePath);
54+
}
55+
56+
// Unknown layout (e.g. pods) — assume template-only.
57+
return true;
58+
}
59+
160
/** @type {import('eslint').Rule.RuleModule} */
261
module.exports = {
362
meta: {
463
type: 'suggestion',
564
docs: {
6-
description: 'disallow this in template-only components (gjs/gts)',
65+
description: 'disallow this in template-only components',
766
category: 'Best Practices',
867
url: 'https://github.com/ember-cli/eslint-plugin-ember/tree/master/docs/rules/template-no-this-in-template-only-components.md',
968
templateMode: 'both',
@@ -40,22 +99,34 @@ module.exports = {
4099
'isDestroyed',
41100
]);
42101

102+
const filename = context.filename;
103+
const isHbs = filename.endsWith('.hbs');
104+
105+
// For .hbs files, check the filesystem for a companion class file.
106+
// If one exists, this is NOT a template-only component — skip entirely.
107+
if (isHbs && !isTemplateOnlyComponent(filename)) {
108+
return {};
109+
}
110+
43111
return {
44112
GlimmerPathExpression(node) {
45-
// Only flag template-only components, not class components.
46-
// Walk ancestors to check if the <template> is inside a class body.
47-
const sourceCode = context.sourceCode;
48-
const ancestors = sourceCode.getAncestors
49-
? sourceCode.getAncestors(node)
50-
: context.getAncestors();
51-
const isInsideClass = ancestors.some(
52-
(ancestor) => ancestor.type === 'ClassBody' || ancestor.type === 'ClassDeclaration'
53-
);
54-
if (isInsideClass) {
55-
return;
113+
// For .gjs/.gts files: walk ancestors to check if <template> is inside a class body.
114+
// If so, the component has a backing class — skip.
115+
// .hbs files are already handled above via the filesystem check.
116+
if (!isHbs) {
117+
const sourceCode = context.sourceCode;
118+
const ancestors = sourceCode.getAncestors
119+
? sourceCode.getAncestors(node)
120+
: context.getAncestors();
121+
const isInsideClass = ancestors.some(
122+
(ancestor) => ancestor.type === 'ClassBody' || ancestor.type === 'ClassDeclaration'
123+
);
124+
if (isInsideClass) {
125+
return;
126+
}
56127
}
57128

58-
// In gjs/gts files with <template> tags, check for this.* usage
129+
// Check for this.* usage
59130
if (node.head?.type === 'ThisHead' && node.tail?.length > 0) {
60131
const original = node.original;
61132
const firstPart = node.tail[0];

‎tests/fixtures/template-no-this-in-template-only-components/app/components/classic-with-class.js‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,3 @@
1+
// Fixture for template-no-this-in-template-only-components rule tests.
2+
// Only needs to exist on disk; the rule's filesystem check uses fs.existsSync.
3+
module.exports = {};

‎tests/fixtures/template-no-this-in-template-only-components/app/components/with-class.js‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,3 @@
1+
// Fixture for template-no-this-in-template-only-components rule tests.
2+
// Only needs to exist on disk; the rule's filesystem check uses fs.existsSync.
3+
module.exports = {};

‎tests/fixtures/template-no-this-in-template-only-components/app/components/with-ts-class.ts‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,3 @@
1+
// Fixture for template-no-this-in-template-only-components rule tests.
2+
// Only needs to exist on disk; the rule's filesystem check uses fs.existsSync.
3+
export {};

‎tests/lib/rules/template-no-this-in-template-only-components.js‎

Lines changed: 81 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,12 @@
1+
const path = require('node:path');
12
const rule = require('../../../lib/rules/template-no-this-in-template-only-components');
23
const RuleTester = require('eslint').RuleTester;
34

5+
const FIXTURES = path.resolve(
6+
__dirname,
7+
'../../fixtures/template-no-this-in-template-only-components'
8+
);
9+
410
const ruleTester = new RuleTester({
511
parser: require.resolve('ember-eslint-parser'),
612
parserOptions: { ecmaVersion: 2022, sourceType: 'module' },
@@ -104,3 +110,78 @@ hbsRuleTester.run('template-no-this-in-template-only-components', rule, {
104110
},
105111
],
106112
});
113+
114+
// Test .hbs files with explicit filenames to verify filesystem-based detection.
115+
// Route templates (app/templates/ but not app/templates/components/) should be skipped.
116+
const hbsFilenameTester = new RuleTester({
117+
parser: require.resolve('ember-eslint-parser/hbs'),
118+
parserOptions: {
119+
ecmaVersion: 2022,
120+
sourceType: 'module',
121+
},
122+
});
123+
124+
hbsFilenameTester.run('template-no-this-in-template-only-components (hbs filenames)', rule, {
125+
valid: [
126+
// Route templates should never be flagged (they always have a controller context)
127+
{
128+
filename: 'app/templates/application.hbs',
129+
code: '{{this.foo}}',
130+
},
131+
// Nested route template path
132+
{
133+
filename: 'app/templates/posts/show.hbs',
134+
code: '{{this.foo}}',
135+
},
136+
// Co-located component with a real .js companion class on disk
137+
{
138+
filename: path.join(FIXTURES, 'app/components/with-class.hbs'),
139+
code: '{{this.foo}}',
140+
},
141+
// Co-located component with a real .ts companion class on disk
142+
{
143+
filename: path.join(FIXTURES, 'app/components/with-ts-class.hbs'),
144+
code: '{{this.foo}}',
145+
},
146+
// Classic structure with a real .js companion class at app/components/<name>.js
147+
{
148+
filename: path.join(FIXTURES, 'app/templates/components/classic-with-class.hbs'),
149+
code: '{{this.foo}}',
150+
},
151+
// Files outside any app/addon directory: cannot determine layout, don't flag
152+
{
153+
filename: '/some/random/path/foo.hbs',
154+
code: '{{this.foo}}',
155+
},
156+
],
157+
invalid: [
158+
// Template-only .hbs component (no companion file on disk — the path doesn't exist)
159+
{
160+
filename: 'app/components/nonexistent-test-component.hbs',
161+
code: '{{this.foo}}',
162+
output: '{{@foo}}',
163+
errors: [{ messageId: 'noThis' }],
164+
},
165+
// Classic structure template-only component (no companion file on disk)
166+
{
167+
filename: 'app/templates/components/nonexistent-test-component.hbs',
168+
code: '{{this.bar}}',
169+
output: '{{@bar}}',
170+
errors: [{ messageId: 'noThis' }],
171+
},
172+
// addon/components: same logic should apply
173+
{
174+
filename: 'addon/components/nonexistent-test-component.hbs',
175+
code: '{{this.foo}}',
176+
output: '{{@foo}}',
177+
errors: [{ messageId: 'noThis' }],
178+
},
179+
// Built-in property (elementId) is not auto-fixable even when companion is missing
180+
{
181+
filename: 'app/components/nonexistent-test-component.hbs',
182+
code: '{{this.elementId}}',
183+
output: null,
184+
errors: [{ messageId: 'noThis' }],
185+
},
186+
],
187+
});

‎vitest.config.mjs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,7 @@ export default defineConfig({
55
globals: true,
66
setupFiles: [],
77
include: ['**/tests/**/*.js'],
8-
exclude: ['tests/helpers/**', 'tests/bench/**', 'node_modules'],
8+
exclude: ['tests/helpers/**', 'tests/bench/**', 'tests/fixtures/**', 'node_modules'],
99
sequence: {
1010
hooks: 'list',
1111
},

0 commit comments

Comments
 (0)