Add new rule template-no-let-reference (#1939)

Co-authored-by: Bryan Mishkin <[email protected]>

Author: patricklx

.eslintignore

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

README.md

@@ -241,6 +241,7 @@ rules in templates can be disabled with eslint directives with mustache or html
 | [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-indent](docs/rules/template-indent.md)                                           | enforce consistent indentation for gts/gjs templates                                                           |    | 🔧 |    |
+| [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,58 @@
+# ember/template-no-let-reference
+
+<!-- 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

@@ -377,20 +377,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)) {
@@ -409,6 +395,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/rules/template-no-let-reference.js

@@ -0,0 +1,43 @@
+/** @type {import('eslint').Rule.RuleModule} */
+module.exports = {
+  meta: {
+    type: 'suggestion',
+    docs: {
+      description: 'disallow referencing let variables in \\<template\\>',
+      category: 'Ember Octane',
+      recommended: false,
+      url: 'https://github.com/ember-cli/eslint-plugin-ember/tree/master/docs/rules/template-no-let-reference.md',
+    },
+    fixable: null,
+    schema: [],
+    messages: {
+      'no-let': 'update-able variables are not supported in templates, reference a const variable',
+    },
+  },
+
+  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, messageId: 'no-let' });
+      }
+    }
+
+    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/lib/rules/template-no-let-reference.js

@@ -0,0 +1,62 @@
+//------------------------------------------------------------------------------
+// Requirements
+//------------------------------------------------------------------------------
+
+const rule = require('../../../lib/rules/template-no-let-reference');
+const RuleTester = require('eslint').RuleTester;
+
+//------------------------------------------------------------------------------
+// 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: rule.meta.messages['no-let'] }],
+    },
+    {
+      code: `
+      var a = '';
+      <template>
+      <a></a>
+      </template>
+      `,
+      output: null,
+      errors: [{ type: 'GlimmerElementNode', message: rule.meta.messages['no-let'] }],
+    },
+  ],
+});