Skip to content
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

fix(eslint-plugin): [no-inputs-metadata-property] do not report on di… #1248

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
99 changes: 99 additions & 0 deletions packages/eslint-plugin/docs/rules/no-inputs-metadata-property.md
Expand Up @@ -489,6 +489,105 @@ class Test {}
class Test {}
```

<br>

---

<br>

#### Default Config

```json
{
"rules": {
"@angular-eslint/no-inputs-metadata-property": [
"error"
]
}
}
```

<br>

#### ✅ Valid Code

```ts
@Component({
selector: 'qx-menuitem',
hostDirectives: [{
directive: CdkMenuItem,
inputs: ['cdkMenuItemDisabled: disabled'],
}]
})
class Test {}
```

<br>

---

<br>

#### Default Config

```json
{
"rules": {
"@angular-eslint/no-inputs-metadata-property": [
"error"
]
}
}
```

<br>

#### ✅ Valid Code

```ts
@Component({
selector: 'qx-menuitem',
'hostDirectives': [{
directive: CdkMenuItem,
inputs: ['cdkMenuItemDisabled: disabled'],
}]
})
class Test {}
```

<br>

---

<br>

#### Default Config

```json
{
"rules": {
"@angular-eslint/no-inputs-metadata-property": [
"error"
]
}
}
```

<br>

#### ✅ Valid Code

```ts
@Component({
selector: 'qx-menuitem',
['hostDirectives']: [{
directive: CdkMenuItem,
inputs: ['cdkMenuItemDisabled: disabled'],
}]
})
class Test {}
```

</details>

<br>
27 changes: 26 additions & 1 deletion packages/eslint-plugin/src/rules/no-inputs-metadata-property.ts
@@ -1,5 +1,6 @@
import { Selectors } from '@angular-eslint/utils';
import { ASTUtils, Selectors } from '@angular-eslint/utils';
import type { TSESTree } from '@typescript-eslint/utils';
import { ASTUtils as TSESLintASTUtils } from '@typescript-eslint/utils';
import { createESLintRule } from '../utils/create-eslint-rule';

type Options = [];
Expand Down Expand Up @@ -29,6 +30,30 @@ export default createESLintRule<Options, MessageIds>({
} ${Selectors.metadataProperty(METADATA_PROPERTY_NAME)}`](
node: TSESTree.Property,
) {
/**
* Angular v15 introduced the directive composition API: https://angular.io/guide/directive-composition-api
* Using host directive inputs using this API is not a bad practice and should not be reported
*/
const ancestorMayBeHostDirectiveAPI = node.parent?.parent?.parent;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

question: I see a difference between the implementation here https://github.com/angular-eslint/angular-eslint/pull/1231/files#diff-34e0047dec14410dccd7fee53b69b0edeb6c62c8b6d4480a4d6144e95aa323b5. What's the reason for it?

Copy link
Contributor Author

@abaran30 abaran30 Dec 7, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only a slight difference 😄 . So in no-input-rename, the targeted selector is Selectors.INPUTS_METADATA_PROPERTY_LITERAL, while the targeted selector for this rule is ${Selectors.COMPONENT_OR_DIRECTIVE_CLASS_DECORATOR} ${Selectors.metadataProperty(METADATA_PROPERTY_NAME)}. Although either of the two selectors could work for this rule, the main reason why I kept the latter selector is because when we do report a lint violation, we want to highlight the inputs keyword like shown in the invalid test cases. When using the former selector, we'd end up highlighting the value bound to inputs, which I think is less helpful.


if (
ancestorMayBeHostDirectiveAPI &&
ASTUtils.isProperty(ancestorMayBeHostDirectiveAPI)
) {
const hostDirectiveAPIPropertyName = 'hostDirectives';

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if (
(ASTUtils.isLiteral(ancestorMayBeHostDirectiveAPI.key) &&
ancestorMayBeHostDirectiveAPI.key.value ===
hostDirectiveAPIPropertyName) ||
(TSESLintASTUtils.isIdentifier(ancestorMayBeHostDirectiveAPI.key) &&
ancestorMayBeHostDirectiveAPI.key.name ===
hostDirectiveAPIPropertyName)
) {
return;
}
}

context.report({
node,
messageId: 'noInputsMetadataProperty',
Expand Down
Expand Up @@ -45,6 +45,41 @@ export const valid = [
})
class Test {}
`,
/**
* Using inputs when using the directive composition API is not a bad practice
* https://angular.io/guide/directive-composition-api
* https://www.youtube.com/watch?v=EJJwyyjsRGs
*/
`

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Component({
selector: 'qx-menuitem',
hostDirectives: [{
directive: CdkMenuItem,
inputs: ['cdkMenuItemDisabled: disabled'],
}]
})
class Test {}
`,
`
@Component({
selector: 'qx-menuitem',
'hostDirectives': [{
directive: CdkMenuItem,
inputs: ['cdkMenuItemDisabled: disabled'],
}]
})
class Test {}
`,
`
@Component({
selector: 'qx-menuitem',
['hostDirectives']: [{
directive: CdkMenuItem,
inputs: ['cdkMenuItemDisabled: disabled'],
}]
})
class Test {}
`,
];

export const invalid = [
Expand Down