-
-
Notifications
You must be signed in to change notification settings - Fork 207
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(eslint-plugin-template): add no duplicate attributes rule #302
feat(eslint-plugin-template): add no duplicate attributes rule #302
Conversation
packages/eslint-plugin-template/src/rules/no-duplicate-attributes.ts
Outdated
Show resolved
Hide resolved
packages/eslint-plugin-template/src/rules/no-duplicate-attributes.ts
Outdated
Show resolved
Hide resolved
packages/eslint-plugin-template/tests/rules/no-duplicate-attributes.test.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the work, @mattlewis92 :) I left my answers to the questions and a feedback with suggestions to further improve the implementation.
packages/eslint-plugin-template/src/rules/no-duplicate-attributes.ts
Outdated
Show resolved
Hide resolved
packages/eslint-plugin-template/src/rules/no-duplicate-attributes.ts
Outdated
Show resolved
Hide resolved
packages/eslint-plugin-template/src/rules/no-duplicate-attributes.ts
Outdated
Show resolved
Hide resolved
packages/eslint-plugin-template/tests/rules/no-duplicate-attributes.test.ts
Outdated
Show resolved
Hide resolved
packages/eslint-plugin-template/tests/rules/no-duplicate-attributes.test.ts
Show resolved
Hide resolved
packages/eslint-plugin-template/src/rules/no-duplicate-attributes.ts
Outdated
Show resolved
Hide resolved
packages/eslint-plugin-template/src/rules/no-duplicate-attributes.ts
Outdated
Show resolved
Hide resolved
packages/eslint-plugin-template/src/rules/no-duplicate-attributes.ts
Outdated
Show resolved
Hide resolved
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
|
9436e1c
to
7627a15
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
packages/eslint-plugin-template/tests/rules/no-duplicate-attributes.test.ts
Show resolved
Hide resolved
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. |
465dad9
to
acdc80a
Compare
acdc80a
to
a8f5c92
Compare
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 😁 |
noDuplicateAttributes: 'Duplicate attribute "{{attributeName}}"', | ||
}, | ||
}, | ||
defaultOptions: [defaultOptions], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: we generally prefer to inline these options:
defaultOptions: [defaultOptions], | |
defaultOptions: [{ allowTwoWayDataBinding: true }], |
], | ||
}), | ||
convertAnnotatedSourceToFailureCase({ | ||
description: 'not allow two way binding and output with the change event', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: what about changing the description to something like this to maintain the standard that was adopted in other cases in this file:
description: 'not allow two way binding and output with the change event', | |
description: 'should fail with two way binding and its corresponding output', |
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!
Let's wait for James' opinion on this. |
Thank you @mattlewis92! |
@rafaelss95 I think your nits were fair enough, feel free to adjust them in future if you want to |
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! 😄