Skip to content

Commit ae66c42

Browse files
NullVoxPopuliNullVoxPopuli
committed
Extract rule: template-no-implicit-this
1 parent 304d6a6 commit ae66c42

4 files changed

Lines changed: 572 additions & 0 deletions

File tree

‎README.md‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -208,6 +208,7 @@ rules in templates can be disabled with eslint directives with mustache or html
208208
| [template-no-chained-this](docs/rules/template-no-chained-this.md) | disallow redundant `this.this` in templates | | 🔧 | |
209209
| [template-no-debugger](docs/rules/template-no-debugger.md) | disallow {{debugger}} in templates | | | |
210210
| [template-no-element-event-actions](docs/rules/template-no-element-event-actions.md) | disallow element event actions (use {{on}} modifier instead) | | | |
211+
| [template-no-implicit-this](docs/rules/template-no-implicit-this.md) | require explicit `this` in property access | | | |
211212
| [template-no-inline-event-handlers](docs/rules/template-no-inline-event-handlers.md) | disallow DOM event handler attributes | | | |
212213
| [template-no-inline-styles](docs/rules/template-no-inline-styles.md) | disallow inline styles | | | |
213214
| [template-no-input-block](docs/rules/template-no-input-block.md) | disallow block usage of {{input}} helper | | | |

‎docs/rules/template-no-implicit-this.md‎

Lines changed: 141 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,141 @@
1+
# ember/template-no-implicit-this
2+
3+
> **HBS Only**: This rule applies to classic `.hbs` template files only (loose mode). It is not relevant for `gjs`/`gts` files (strict mode), where these patterns cannot occur.
4+
5+
<!-- end auto-generated rule header -->
6+
7+
Require explicit `this` for property access in templates to avoid ambiguity.
8+
9+
This rule aides in the migration path for [emberjs/rfcs#308](https://github.com/emberjs/rfcs/pull/308).
10+
11+
## Motivation
12+
13+
Currently, the way to access properties on a components class is `{{greeting}}`
14+
from a template. This works because the component class is one of the objects
15+
we resolve against during the evaluation of the expression.
16+
17+
The first problem with this approach is that the `{{greeting}}` syntax is
18+
ambiguous, as it could be referring to a local variable (block param), a helper
19+
with no arguments, a closed over component, or a property on the component
20+
class.
21+
22+
Consider the following example where the ambiguity can cause issues:
23+
24+
You have a component class that looks like the following component and template:
25+
26+
```js
27+
import Component from '@ember/component';
28+
import computed from '@ember/computed';
29+
30+
export default Component.extend({
31+
formatName: computed('firstName', 'lastName', function () {
32+
return `${this.firstName} ${this.lastName}`;
33+
}),
34+
});
35+
```
36+
37+
```hbs
38+
<h1>Hello {{formatName}}!</h1>
39+
```
40+
41+
Given `{ firstName: 'Chad', lastName: 'Hietala' }`, Ember will render the
42+
following:
43+
44+
```html
45+
<h1>Hello Chad Hietala!</h1>
46+
```
47+
48+
Now some time goes on and someone adds a `formatName` helper at
49+
`app/helpers/formatName.js` that looks like the following:
50+
51+
```js
52+
export default function formatName([firstName, lastName]) {
53+
return `${firstName} ${lastName}`;
54+
}
55+
```
56+
57+
Due to the fact that helpers take precedence over property lookups, our
58+
`{{formatName}}` now resolves to a helper. When the helper runs it doesn't have
59+
any arguments so our template now renders the following:
60+
61+
```html
62+
<h1>Hello !</h1>
63+
```
64+
65+
This can be a refactoring hazard and can often lead to confusion for readers of
66+
the template. Upon encountering `{{greeting}}` in a component's template, the
67+
reader has to check all of these places: first, you need to scan the
68+
surrounding lines for block params with that name; next, you check in the
69+
helpers folder to see if there is a helper with that name (it could also be
70+
coming from an addon!); finally, you check the component's JavaScript class to
71+
look for a (computed) property.
72+
73+
Like
74+
[RFC#0276](https://github.com/emberjs/rfcs/blob/master/text/0276-named-args.md)
75+
made argument usage explicit through the `@` prefix, the `this` prefix will
76+
resolve the ambiguity and greatly improve clarity, especially in big projects
77+
with a lot of files (and uses a lot of addons).
78+
79+
As an aside, the ambiguity that causes confusion for human readers is also a
80+
problem for the compiler. While it is not the main goal of this proposal,
81+
resolving this ambiguity also helps the rendering system. Currently, the
82+
"runtime" template compiler has to perform a helper lookup for every
83+
`{{greeting}}` in each template. It will be able to skip this resolution
84+
process and perform other optimizations (such as reusing the internal
85+
[reference](https://github.com/glimmerjs/glimmer-vm/blob/master/guides/04-references.md)
86+
object and caches) with this addition.
87+
88+
Furthermore, by enforcing the `this` prefix, tooling like the [Ember Language
89+
Server](https://github.com/emberwatch/ember-language-server) does not need to
90+
know about fallback resolution rules. This makes common features like ["Go To
91+
Definition"](https://code.visualstudio.com/docs/editor/editingevolved#_go-to-definition)
92+
much easier to implement since we have semantics that mean "property on class".
93+
94+
## Examples
95+
96+
Examples of **incorrect** code for this rule:
97+
98+
```hbs
99+
{{property}}
100+
{{someValue}}
101+
```
102+
103+
Examples of **correct** code for this rule:
104+
105+
```hbs
106+
{{this.property}}
107+
{{@namedArg}}
108+
{{yield}}
109+
{{if this.condition 'yes' 'no'}}
110+
```
111+
112+
## Options
113+
114+
- object -- An object with the following keys:
115+
- `allow` -- An array of string names to allow as implicit this. Paths that exactly match an entry in this list will not be flagged.
116+
117+
Example:
118+
119+
```json
120+
{
121+
"ember/template-no-implicit-this": [
122+
"error",
123+
{
124+
"allow": ["book-details"]
125+
}
126+
]
127+
}
128+
```
129+
130+
## Migration
131+
132+
- use [ember-no-implicit-this-codemod](https://github.com/ember-codemods/ember-no-implicit-this-codemod)
133+
- [upgrade to Glimmer components](https://guides.emberjs.com/release/upgrading/current-edition/glimmer-components/), which don't allow ambiguous access
134+
- classic components have [auto-reflection](https://github.com/emberjs/rfcs/blob/master/text/0276-named-args.md#motivation), and can use `this.myArgName` or `this.args.myArgNme` or `@myArgName` interchangeably
135+
136+
## References
137+
138+
- [Glimmer components](https://guides.emberjs.com/release/upgrading/current-edition/glimmer-components/)
139+
- [RFC#0276 - Named Args](https://github.com/emberjs/rfcs/blob/master/text/0276-named-args.md#motivation)
140+
- [RFC#308 - Deprecate implicit this](https://github.com/emberjs/rfcs/blob/master/text/0308-deprecate-property-lookup-fallback.md)
141+
- [Ember Octane Guide - Templates](https://guides.emberjs.com/release/components/component-state-and-actions/)

‎lib/rules/template-no-implicit-this.js‎

Lines changed: 159 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,159 @@
1+
// Built-in helpers and keywords
2+
const BUILT_INS = new Set([
3+
'yield',
4+
'outlet',
5+
'has-block',
6+
'has-block-params',
7+
'hasBlock',
8+
'hasBlockParams',
9+
'if',
10+
'unless',
11+
'each',
12+
'let',
13+
'with',
14+
'each-in',
15+
'concat',
16+
'get',
17+
'array',
18+
'hash',
19+
'log',
20+
'debugger',
21+
'component',
22+
'helper',
23+
'modifier',
24+
'mount',
25+
'input',
26+
'textarea',
27+
'query-params',
28+
'unique-id',
29+
]);
30+
31+
// Control-flow built-ins whose params should not be flagged
32+
const CONTROL_FLOW_HELPERS = new Set([
33+
'if',
34+
'unless',
35+
'each',
36+
'let',
37+
'with',
38+
'each-in',
39+
'concat',
40+
'get',
41+
'array',
42+
'hash',
43+
'log',
44+
]);
45+
46+
function isMustacheCalleeWithArgs(node) {
47+
const parent = node.parent;
48+
if (parent.path !== node) {
49+
return false;
50+
}
51+
if (parent.params && parent.params.length > 0) {
52+
return true;
53+
}
54+
return Boolean(parent.hash && parent.hash.pairs && parent.hash.pairs.length > 0);
55+
}
56+
57+
function isControlFlowParam(node) {
58+
const callee = node.parent.path?.original;
59+
return CONTROL_FLOW_HELPERS.has(callee) && node.parent.params?.includes(node);
60+
}
61+
62+
function isBlockParamPath(node, path) {
63+
const blockParams = node.parent.program?.blockParams || [];
64+
return blockParams.includes(path.split('.')[0]);
65+
}
66+
67+
/** @type {import('eslint').Rule.RuleModule} */
68+
module.exports = {
69+
meta: {
70+
type: 'suggestion',
71+
docs: {
72+
description: 'require explicit `this` in property access',
73+
category: 'Best Practices',
74+
url: 'https://github.com/ember-cli/eslint-plugin-ember/tree/master/docs/rules/template-no-implicit-this.md',
75+
templateMode: 'loose',
76+
},
77+
schema: [
78+
{
79+
type: 'object',
80+
properties: {
81+
allow: {
82+
type: 'array',
83+
items: { type: 'string' },
84+
uniqueItems: true,
85+
},
86+
},
87+
additionalProperties: false,
88+
},
89+
],
90+
messages: {
91+
noImplicitThis:
92+
'Ambiguous path "{{path}}" is not allowed. Use "@{{path}}" if it is a named argument or "this.{{path}}" if it is a property on the component.',
93+
},
94+
originallyFrom: {
95+
name: 'ember-template-lint',
96+
rule: 'lib/rules/no-implicit-this.js',
97+
docs: 'docs/rule/no-implicit-this.md',
98+
tests: 'test/unit/rules/no-implicit-this-test.js',
99+
},
100+
},
101+
102+
create(context) {
103+
const allowList = context.options[0]?.allow || [];
104+
105+
return {
106+
GlimmerPathExpression(node) {
107+
const path = node.original;
108+
109+
// Skip if path starts with @ (named arg) or this. (explicit)
110+
if (path.startsWith('@') || path.startsWith('this.')) {
111+
return;
112+
}
113+
114+
// Skip built-in helpers and keywords
115+
if (BUILT_INS.has(path)) {
116+
return;
117+
}
118+
119+
// Skip paths matching the allow list (exact match only)
120+
if (allowList.includes(path)) {
121+
return;
122+
}
123+
124+
// Skip single identifiers that are the callee of a helper-like MustacheStatement
125+
if (node.parent && node.parent.type === 'GlimmerMustacheStatement') {
126+
if (isMustacheCalleeWithArgs(node)) {
127+
return;
128+
}
129+
if (isControlFlowParam(node)) {
130+
return;
131+
}
132+
}
133+
134+
// Skip paths that are part of block params
135+
if (node.parent && node.parent.type === 'GlimmerBlockStatement') {
136+
if (isBlockParamPath(node, path)) {
137+
return;
138+
}
139+
}
140+
141+
// Report ambiguous paths that should use this. or @
142+
if (!path.includes('.') || !path.startsWith('this.')) {
143+
const firstPart = path.split('.')[0];
144+
145+
// Skip if it looks like a component (PascalCase)
146+
if (firstPart[0] === firstPart[0].toUpperCase()) {
147+
return;
148+
}
149+
150+
context.report({
151+
node,
152+
messageId: 'noImplicitThis',
153+
data: { path },
154+
});
155+
}
156+
},
157+
};
158+
},
159+
};

0 commit comments

Comments
 (0)