fix(eslint-plugin-template): [attributes-order] fixes for structural …

…directives and "dotted" names (#1448)
angular-eslint · Jul 11, 2023 · 90c0e91 · 90c0e91
1 parent 02c2a97
commit 90c0e91
Show file tree

Hide file tree

Showing 3 changed files with 319 additions and 13 deletions.
diff --git a/packages/eslint-plugin-template/docs/rules/attributes-order.md b/packages/eslint-plugin-template/docs/rules/attributes-order.md
@@ -109,6 +109,36 @@ interface Options {
 
 <br>
 
+#### Custom Config
+
+```json
+{
+  "rules": {
+    "@angular-eslint/template/attributes-order": [
+      "error",
+      {
+        "alphabetical": true
+      }
+    ]
+  }
+}
+```
+
+<br>
+
+#### ❌ Invalid Code
+
+```html
+<li><input type="text" id="input"></li>
+           ~~~~~~~~~~~~~~~~~~~~~~
+```
+
+<br>
+
+---
+
+<br>
+
 #### Default Config
 
 ```json
@@ -339,6 +369,144 @@ interface Options {
   ~~~~~~~~~~~~~~~~
 ```
 
+<br>
+
+---
+
+<br>
+
+#### Default Config
+
+```json
+{
+  "rules": {
+    "@angular-eslint/template/attributes-order": [
+      "error"
+    ]
+  }
+}
+```
+
+<br>
+
+#### ❌ Invalid Code
+
+```html
+<ng-container (click)="bar = []" id="issue" *ngFor="let foo of bar"></ng-container>
+              ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+```
+
+<br>
+
+---
+
+<br>
+
+#### Default Config
+
+```json
+{
+  "rules": {
+    "@angular-eslint/template/attributes-order": [
+      "error"
+    ]
+  }
+}
+```
+
+<br>
+
+#### ❌ Invalid Code
+
+```html
+<ng-container (click)="bar = []" id="issue" *ngFor="let foo of bar; index as i; first as isFirst"></ng-container>
+              ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+```
+
+<br>
+
+---
+
+<br>
+
+#### Default Config
+
+```json
+{
+  "rules": {
+    "@angular-eslint/template/attributes-order": [
+      "error"
+    ]
+  }
+}
+```
+
+<br>
+
+#### ❌ Invalid Code
+
+```html
+<div id="id" *ngIf="bar as foo"></div>
+     ~~~~~~~~~~~~~~~~~~~~~~~~~~
+```
+
+<br>
+
+---
+
+<br>
+
+#### Default Config
+
+```json
+{
+  "rules": {
+    "@angular-eslint/template/attributes-order": [
+      "error"
+    ]
+  }
+}
+```
+
+<br>
+
+#### ❌ Invalid Code
+
+```html
+<div id="id" *ngIf="condition then foo else bar"></div>
+     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+```
+
+<br>
+
+---
+
+<br>
+
+#### Custom Config
+
+```json
+{
+  "rules": {
+    "@angular-eslint/template/attributes-order": [
+      "error",
+      {
+        "alphabetical": true
+      }
+    ]
+  }
+}
+```
+
+<br>
+
+#### ❌ Invalid Code
+
+```html
+<div [disabled]="disabled" [class.disabled]="disabled"></div>
+     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+```
+
 </details>
 
 <br>

diff --git a/packages/eslint-plugin-template/src/rules/attributes-order.ts b/packages/eslint-plugin-template/src/rules/attributes-order.ts
@@ -6,7 +6,10 @@ import type {
   TmplAstTextAttribute,
   TmplAstNode,
 } from '@angular-eslint/bundled-angular-compiler';
-import { TmplAstTemplate } from '@angular-eslint/bundled-angular-compiler';
+import {
+  ParseSourceSpan,
+  TmplAstTemplate,
+} from '@angular-eslint/bundled-angular-compiler';
 import { getTemplateParserServices } from '@angular-eslint/utils';
 import type { TSESTree } from '@typescript-eslint/utils';
 import { createESLintRule } from '../utils/create-eslint-rule';
@@ -227,7 +230,10 @@ function byOrder(order: readonly OrderType[], alphabetical: boolean) {
       getOrderIndex(one, order) - getOrderIndex(other, order);
 
     if (alphabetical && orderComparison === 0) {
-      return one.name > other.name ? 1 : -1;
+      return (one.keySpan?.details ?? one.name) >
+        (other.keySpan?.details ?? other.name)
+        ? 1
+        : -1;
     }
 
     return orderComparison;
@@ -280,9 +286,36 @@ function toTemplateReferenceVariableOrderType(reference: TmplAstReference) {
 function extractTemplateAttrs(
   node: ExtendedTmplAstElement,
 ): (ExtendedTmplAstBoundAttribute | ExtendedTmplAstTextAttribute)[] {
-  return isTmplAstTemplate(node.parent)
-    ? node.parent.templateAttrs.map(toStructuralDirectiveOrderType)
-    : [];
+  if (!isTmplAstTemplate(node.parent)) {
+    return [];
+  }
+
+  /*
+   * There may be multiple "attributes" for a structural directive even though
+   * there is only a single HTML attribute:
+   * e.g. `<ng-container *ngFor="let foo of bar"></ng-container>`
+   * will parsed as two attributes (`ngFor` and `ngForOf`)
+   */
+
+  const attrs = node.parent.templateAttrs.map(toStructuralDirectiveOrderType);
+
+  // Pick up on any subsequent `let` bindings, e.g. `index as i`
+  let sourceEnd = attrs[attrs.length - 1].sourceSpan.end;
+  node.parent.variables.forEach((v) => {
+    if (
+      v.sourceSpan.start.offset <= sourceEnd.offset &&
+      sourceEnd.offset < v.sourceSpan.end.offset
+    ) {
+      sourceEnd = v.sourceSpan.end;
+    }
+  });
+
+  return [
+    {
+      ...attrs[0],
+      sourceSpan: new ParseSourceSpan(attrs[0].sourceSpan.start, sourceEnd),
+    } as ExtendedTmplAstBoundAttribute | ExtendedTmplAstTextAttribute,
+  ];
 }
 
 function normalizeInputsOutputs(
@@ -333,19 +366,20 @@ function isOnSameLocation(
 }
 
 function getMessageName(expected: ExtendedAttribute): string {
+  const fullName = expected.keySpan?.details ?? expected.name;
   switch (expected.orderType) {
     case OrderType.StructuralDirective:
-      return `*${expected.name}`;
+      return `*${fullName}`;
     case OrderType.TemplateReferenceVariable:
-      return `#${expected.name}`;
+      return `#${fullName}`;
     case OrderType.InputBinding:
-      return `[${expected.name}]`;
+      return `[${fullName}]`;
     case OrderType.OutputBinding:
-      return `(${expected.name})`;
+      return `(${fullName})`;
     case OrderType.TwoWayBinding:
-      return `[(${expected.name})]`;
+      return `[(${fullName})]`;
     default:
-      return expected.name;
+      return fullName;
   }
 }
 

diff --git a/packages/eslint-plugin-template/tests/rules/attributes-order/cases.ts b/packages/eslint-plugin-template/tests/rules/attributes-order/cases.ts
@@ -1,6 +1,8 @@
 import { convertAnnotatedSourceToFailureCase } from '@angular-eslint/utils';
-import type { MessageIds } from '../../../src/rules/attributes-order';
-import { OrderType } from '../../../src/rules/attributes-order';
+import {
+  OrderType,
+  type MessageIds,
+} from '../../../src/rules/attributes-order';
 
 const messageId: MessageIds = 'attributesOrder';
 
@@ -14,6 +16,27 @@ export const valid = [
 ];
 
 export const invalid = [
+  convertAnnotatedSourceToFailureCase({
+    messageId,
+    description: 'should work for simple attributes',
+    annotatedSource: `
+      <li><input type="text" id="input"></li>
+                 ~~~~~~~~~~~~~~~~~~~~~~
+    `,
+    options: [
+      {
+        alphabetical: true,
+      },
+    ],
+    data: {
+      expected: '`id`, `type`',
+      actual: '`type`, `id`',
+    },
+    annotatedOutput: `
+      <li><input id="input" type="text"></li>
+                 
+    `,
+  }),
   convertAnnotatedSourceToFailureCase({
     messageId,
     description: 'should fail if structural directive is not first',
@@ -195,4 +218,85 @@ export const invalid = [
         
     `,
   }),
+  convertAnnotatedSourceToFailureCase({
+    messageId,
+    description: 'should work for ngFor',
+    annotatedSource: `
+      <ng-container (click)="bar = []" id="issue" *ngFor="let foo of bar"></ng-container>
+                    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+    `,
+    data: {
+      expected: '`*ngFor`, `id`, `(click)`',
+      actual: '`(click)`, `id`, `*ngFor`',
+    },
+    annotatedOutput: `
+      <ng-container *ngFor="let foo of bar" id="issue" (click)="bar = []"></ng-container>
+                    
+    `,
+  }),
+  convertAnnotatedSourceToFailureCase({
+    messageId,
+    description: 'should work for ngFor with let',
+    annotatedSource: `
+      <ng-container (click)="bar = []" id="issue" *ngFor="let foo of bar; index as i; first as isFirst"></ng-container>
+                    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+    `,
+    data: {
+      expected: '`*ngFor`, `id`, `(click)`',
+      actual: '`(click)`, `id`, `*ngFor`',
+    },
+    annotatedOutput: `
+      <ng-container *ngFor="let foo of bar; index as i; first as isFirst" id="issue" (click)="bar = []"></ng-container>
+                    
+    `,
+  }),
+  convertAnnotatedSourceToFailureCase({
+    messageId,
+    description: 'should work for ngIf as',
+    annotatedSource: `
+      <div id="id" *ngIf="bar as foo"></div>
+           ~~~~~~~~~~~~~~~~~~~~~~~~~~
+    `,
+    data: {
+      expected: '`*ngIf`, `id`',
+      actual: '`id`, `*ngIf`',
+    },
+    annotatedOutput: `
+      <div *ngIf="bar as foo" id="id"></div>
+           
+    `,
+  }),
+  convertAnnotatedSourceToFailureCase({
+    messageId,
+    description: 'should work for ngIfThenElse',
+    annotatedSource: `
+      <div id="id" *ngIf="condition then foo else bar"></div>
+           ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+    `,
+    data: {
+      expected: '`*ngIf`, `id`',
+      actual: '`id`, `*ngIf`',
+    },
+    annotatedOutput: `
+      <div *ngIf="condition then foo else bar" id="id"></div>
+           
+    `,
+  }),
+  convertAnnotatedSourceToFailureCase({
+    messageId,
+    description: 'should work for dotted attributes',
+    annotatedSource: `
+      <div [disabled]="disabled" [class.disabled]="disabled"></div>
+           ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+    `,
+    options: [{ alphabetical: true }],
+    data: {
+      expected: '`[class.disabled]`, `[disabled]`',
+      actual: '`[disabled]`, `[class.disabled]`',
+    },
+    annotatedOutput: `
+      <div [class.disabled]="disabled" [disabled]="disabled"></div>
+           
+    `,
+  }),
 ];