add template-no-let-reference rule

Author: patricklx

.eslintignore

@@ -3,4 +3,5 @@ node_modules
 lib/recommended-rules.js
 
 # Contains <template> in js markdown
-docs/rules/no-empty-glimmer-component-classes.md
+docs/rules/no-empty-glimmer-component-classes.md
+docs/rules/template-no-let-reference.md

README.md

@@ -164,6 +164,7 @@ module.exports = {
 | [no-ember-super-in-es-classes](docs/rules/no-ember-super-in-es-classes.md)                 | disallow use of `this._super` in ES class methods                                                              | ✅  | 🔧 |    |
 | [no-empty-glimmer-component-classes](docs/rules/no-empty-glimmer-component-classes.md)     | disallow empty backing classes for Glimmer components                                                          | ✅  |    |    |
 | [no-tracked-properties-from-args](docs/rules/no-tracked-properties-from-args.md)           | disallow creating @tracked properties from this.args                                                           | ✅  |    |    |
+| [template-no-let-reference](docs/rules/template-no-let-reference.md)                       | disallow referencing let variables in \<template\>                                                             | ✅  |    |    |
 
 ### jQuery
 

docs/rules/template-no-let-reference.md

@@ -0,0 +1,60 @@
+# ember/template-no-let-reference
+
+💼 This rule is enabled in the ✅ `recommended` [config](https://github.com/ember-cli/eslint-plugin-ember#-configurations).
+
+<!-- end auto-generated rule header -->
+
+Disallows referencing let/var variables in templates.
+
+```js
+// app/components/foo.gjs
+let foo = 1;
+
+function increment() {
+foo++;
+}
+
+export default <template>{{foo}}</template>;
+````
+
+This does not "work" – it doesn't error, but it will essentially capture and compile in the value of the foo variable at some arbitrary point and never update again. Even if the component is torn down and a new instance is created/rendered, it will probably still hold on to the old value when the template was initially compiled.
+
+So, generally speaking, one should avoid referencing let variables from within &lt;template&gt; and instead prefer to use const bindings.
+
+## Rule Detail
+
+Use `const` variables instead of `let`.
+
+## Examples
+
+Examples of **incorrect** code for this rule:
+
+```js
+let x = 1;
+<template>
+  {{x}}
+</template>
+```
+
+```js
+let Comp = x; // SomeComponent
+<template>
+  <Comp />
+</template>
+```
+
+Examples of **correct** code for this rule:
+
+```js
+const x = 1;
+<template>
+  {{x}}
+</template>
+```
+
+```js
+const Comp = x; // SomeComponent
+<template>
+  <Comp />
+</template>
+```

lib/parsers/gjs-gts-parser.js

@@ -368,20 +368,6 @@ function convertAst(result, preprocessedResult, visitorKeys) {
       Object.assign(node, ast);
     }
 
-    if ('blockParams' in node) {
-      const upperScope = findParentScope(result.scopeManager, path);
-      const scope = result.isTypescript
-        ? new TypescriptScope.BlockScope(result.scopeManager, upperScope, node)
-        : new Scope(result.scopeManager, 'block', upperScope, node);
-      for (const [i, b] of node.params.entries()) {
-        const v = new Variable(b.name, scope);
-        v.identifiers.push(b);
-        v.defs.push(new Definition('Parameter', b, node, node, i, 'Block Param'));
-        scope.variables.push(v);
-        scope.set.set(b.name, v);
-      }
-    }
-
     if (node.type === 'GlimmerPathExpression' && node.head.type === 'VarHead') {
       const name = node.head.name;
       if (glimmer.isKeyword(name)) {
@@ -400,6 +386,20 @@ function convertAst(result, preprocessedResult, visitorKeys) {
         registerNodeInScope(node, scope, variable);
       }
     }
+
+    if ('blockParams' in node) {
+      const upperScope = findParentScope(result.scopeManager, path);
+      const scope = result.isTypescript
+        ? new TypescriptScope.BlockScope(result.scopeManager, upperScope, node)
+        : new Scope(result.scopeManager, 'block', upperScope, node);
+      for (const [i, b] of node.params.entries()) {
+        const v = new Variable(b.name, scope);
+        v.identifiers.push(b);
+        v.defs.push(new Definition('Parameter', b, node, node, i, 'Block Param'));
+        scope.variables.push(v);
+        scope.set.set(b.name, v);
+      }
+    }
     return null;
   });
 

lib/recommended-rules.js

@@ -73,6 +73,7 @@ module.exports = {
   "ember/require-tagless-components": "error",
   "ember/require-valid-css-selector-in-test-helpers": "error",
   "ember/routes-segments-snake-case": "error",
+  "ember/template-no-let-reference": "error",
   "ember/use-brace-expansion": "error",
   "ember/use-ember-data-rfc-395-imports": "error"
 }

lib/rules/template-no-let-reference.js

@@ -0,0 +1,44 @@
+const ERROR_MESSAGE =
+  'update-able variables are not supported in templates, reference a const variable';
+
+/** @type {import('eslint').Rule.RuleModule} */
+module.exports = {
+  ERROR_MESSAGE,
+  meta: {
+    type: 'suggestion',
+    docs: {
+      description: 'disallow referencing let variables in \\<template\\>',
+      category: 'Ember Octane',
+      recommended: true,
+      url: 'https://github.com/ember-cli/eslint-plugin-ember/tree/master/docs/rules/template-no-let-reference.md',
+    },
+    fixable: null,
+    schema: [],
+  },
+
+  create: (context) => {
+    const sourceCode = context.sourceCode;
+
+    function checkIfWritableReferences(node, scope) {
+      const ref = scope.references.find((v) => v.identifier === node);
+      if (!ref) {
+        return;
+      }
+      if (ref.resolved?.identifiers.some((i) => ['let', 'var'].includes(i.parent.parent.kind))) {
+        context.report({ node, message: ERROR_MESSAGE });
+      }
+    }
+
+    return {
+      GlimmerPathExpression(node) {
+        checkIfWritableReferences(node.head, sourceCode.getScope(node));
+      },
+
+      GlimmerElementNode(node) {
+        // glimmer element is in its own scope, need tp get upper scope
+        const scope = sourceCode.getScope(node.parent);
+        checkIfWritableReferences(node, scope);
+      },
+    };
+  },
+};

tests/__snapshots__/recommended.js.snap

@@ -70,6 +70,7 @@ exports[`recommended rules has the right list 1`] = `
   "require-tagless-components",
   "require-valid-css-selector-in-test-helpers",
   "routes-segments-snake-case",
+  "template-no-let-reference",
   "use-brace-expansion",
   "use-ember-data-rfc-395-imports",
 ]

tests/lib/rules/template-no-let-reference.js

@@ -0,0 +1,64 @@
+//------------------------------------------------------------------------------
+// Requirements
+//------------------------------------------------------------------------------
+
+const rule = require('../../../lib/rules/template-no-let-reference');
+const RuleTester = require('eslint').RuleTester;
+
+const { ERROR_MESSAGE } = rule;
+
+//------------------------------------------------------------------------------
+// Tests
+//------------------------------------------------------------------------------
+
+const ruleTester = new RuleTester({
+  parser: require.resolve('../../../lib/parsers/gjs-gts-parser.js'),
+  parserOptions: { ecmaVersion: 2020, sourceType: 'module' },
+});
+ruleTester.run('template-no-let-reference', rule, {
+  valid: [
+    `
+      const a = '';
+      <template>
+      {{a}}
+      </template>
+    `,
+    `
+      const a = '';
+      <template>
+      <a></a>
+      </template>
+    `,
+    // make sure rule does not error out on missing references
+    `
+      const a = '';
+      <template>
+      {{ab}}
+      <ab></ab>
+      </template>
+    `,
+  ],
+
+  invalid: [
+    {
+      code: `
+      let a = '';
+      <template>
+      {{a}}
+      </template>
+      `,
+      output: null,
+      errors: [{ type: 'VarHead', message: ERROR_MESSAGE }],
+    },
+    {
+      code: `
+      var a = '';
+      <template>
+      <a></a>
+      </template>
+      `,
+      output: null,
+      errors: [{ type: 'GlimmerElementNode', message: ERROR_MESSAGE }],
+    },
+  ],
+});