feat(eslint-plugin-template): add no duplicate attributes rule

[mattlewis92]

This PR implements the rule suggested in #293. This was my first time writing an eslint rule, so apologies if I made any silly mistakes 😄 It was a really nice codebase to contribute to with lots of examples so hopefully there is nothing too bad in there. I tried to highlight the things I wasn't sure about, please let me know if you'd like me to make any changes! 😄

[rafaelss95]

Thanks for the work, @mattlewis92 :) I left my answers to the questions and a feedback with suggestions to further improve the implementation.

[mattlewis92]

Thanks for the work, @mattlewis92 :) I left my answers to the questions and a feedback with suggestions to further improve the implementation.

Awesome, thank you so much! I will get to work on those comments in the next few days.

Some more test cases I found when testing this on our app which need handling

(@flyInOut.done)="animationDone($event)"
(@flyInOut.start)="animationStarted($event)"

class="batch-merge-tasks__dropdown-merge-button"
[class.disabled]="!selectedMergeTask"
[disabled]="!selectedMergeTask"

[style.width.px]="col.width"
[width]="col.width"

(window:keydown)="manageCreateKeys($event)"
(document:keydown)="checkCreateText($event)"
(document:keyup)="checkCreateText($event); preventBadKeys($event)"
(keyup)="preventBadKeys($event)"
(keydown)="preventBadKeys($event)"

[mattlewis92]

I've addressed most of the review comments + fixed some bugs that I found when testing this rule on our app. The outstanding things are:

1/ decide on the best name for the rule https://github.com/angular-eslint/angular-eslint/pull/302/files#r559227915
2/ add option for treating <input [(ngModel)]="model" (ngModelChange)="model = $event"> as an error (I can't think of a good name for the option) https://github.com/angular-eslint/angular-eslint/pull/302/files#r559228372
3/ should we report duplicate errors for each attribute? https://github.com/angular-eslint/angular-eslint/pull/302/files#r559228851
4/ [style.foo] and [class.foo] don't appear to be properly represented in the AST so we need a way to workaround that https://github.com/angular-eslint/angular-eslint/pull/302/files#r563320986

[rafaelss95]

Hey, while I was testing some scenarios, I discovered that the compiler doesn't throw error for duplicate variables.

Case 1 (duplicate template reference variables on the same element):

<input #inputRef type="number" inputmode="numeric" #inputRef>

Case 2 (duplicate template reference variables on the same template):

<div #divRef></div>
<div #divRef></div>

Case 3 (duplicate scoped variables on the same template):

<div *ngFor="let item of [1, 2]; index as index; odd as odd; index as index"></div>

Another case is a possible conflict with a component property:

@Component({
  template: `
    <input #test>
    {{ test }}
  `
})
class TestComponent {
  test = 2;
}

I've reported it in angular/angular#40536, however the available fix seems to fix the first case only. That being said, I'm not sure it's still in the scope of this rule, since we're dealing with duplicates or maybe it's a new rule proposal. Anyway, I thought it was important to leave it here.

[mattlewis92]

Hey, while I was testing some scenarios, I discovered that the compiler doesn't throw error for duplicate variables.

Case 1 (duplicate template reference variables on the same element):

<input #inputRef type="number" inputmode="numeric" #inputRef>

Case 2 (duplicate template reference variables on the same template):

<div #divRef></div>
<div #divRef></div>

Case 3 (duplicate scoped variables on the same template):

<div *ngFor="let item of [1, 2]; index as index; odd as odd; index as index"></div>

Another case is a possible conflict with a component property:

@Component({
  template: `
    <input #test>
    {{ test }}
  `
})
class TestComponent {
  test = 2;
}

I've reported it in angular/angular#40536, however the available fix seems to fix the first case only. That being said, I'm not sure it's still in the scope of this rule, since we're dealing with duplicates or maybe it's a new rule proposal. Anyway, I thought it was important to leave it here.

Oh that is a great point, cases 2 + 4 I've seen a lot of problems in the wild caused by those issues. I think it would make sense as a separate rule though, or even for the angular compiler to just handle all of those cases.

[mattlewis92]

I've fixed the failing test cases and added the option to disallow two way binding + an output with the same name. We just need to decide on the best name for the rule https://github.com/angular-eslint/angular-eslint/pull/302/files#r559227915 and whether to report errors for each attribute: https://github.com/angular-eslint/angular-eslint/pull/302/files#r559228851 and then this should be ready to go 😁

[rafaelss95]

nit: we generally prefer to inline these options:

Suggested change

	defaultOptions: [defaultOptions],
	defaultOptions: [{ allowTwoWayDataBinding: true }],

[rafaelss95]

nit: what about changing the description to something like this to maintain the standard that was adopted in other cases in this file:

Suggested change

	description: 'not allow two way binding and output with the change event',
	description: 'should fail with two way binding and its corresponding output',

[rafaelss95]

I left some nits, but everything in general looks great to me, especially the tests that I really liked the granularity 😛. Really good work, @mattlewis92, thanks!

I've fixed the failing test cases and added the option to disallow two way binding + an output with the same name. We just need to decide on the best name for the rule https://github.com/angular-eslint/angular-eslint/pull/302/files#r559227915 and whether to report errors for each attribute: https://github.com/angular-eslint/angular-eslint/pull/302/files#r559228851 and then this should be ready to go 😁

Let's wait for James' opinion on this.

[JamesHenry]

Thank you @mattlewis92!

[JamesHenry]

@rafaelss95 I think your nits were fair enough, feel free to adjust them in future if you want to