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

Target property with decorator #662

Closed
jerone opened this issue Dec 24, 2020 · 5 comments
Closed

Target property with decorator #662

jerone opened this issue Dec 24, 2020 · 5 comments
Labels

Comments

@jerone
Copy link

jerone commented Dec 24, 2020

How can I make the rule jsdoc/require-jsdoc work with properties that have a specific decorator/annotation in TypeScript?

Configuration: https://repl.it/@jerone/eslint-jsdoc-requiredoc-decorator#.eslintrc.json

{
  "parser": "@typescript-eslint/parser",
  "plugins": ["jsdoc"],
  "rules": {
    "jsdoc/require-jsdoc": [
      "error",
      {
        "contexts": ["Property:has(Decorator[name=\"Input\"])"]
      }
    ]
  }
}
export class User {
  @Input()
  public name: string;
}
@brettz9
Copy link
Collaborator

brettz9 commented Jan 3, 2021

As per the docs, you can explore the AST, being sure to specify your parser (in this case @typescript-eslint/parser) at https://astexplorer.net/

Using an esquery expression such as ESLint uses, this appears to work:

ClassProperty:has(Decorator[expression.callee.name="Input"])

Closing as that should resolve, but feel free to comment further as needed.

brettz9 added a commit to brettz9/eslint-plugin-jsdoc that referenced this issue Jan 3, 2021
@brettz9
Copy link
Collaborator

brettz9 commented Jan 3, 2021

Note that the jsdoc block will be expected above the decorator.

@brettz9 brettz9 closed this as completed Jan 3, 2021
@jerone
Copy link
Author

jerone commented Jan 31, 2021

Thank you @brettz9 for your answer. I finally had time to test this, but it appears to not be working correctly.

Setup

My config looks like this:

{
  "parser": "@typescript-eslint/parser",
  "plugins": ["jsdoc"],
  "rules": {
    "jsdoc/require-jsdoc": [
      "warn",
      {
        "require": {
          "ArrowFunctionExpression": true,
          "ClassDeclaration": false, // Overridden below.
          "ClassExpression": true,
          "FunctionDeclaration": true,
          "FunctionExpression": true,
          "MethodDefinition": false // Overridden below.
        },
        "checkConstructors": false,
        "checkGetters": true,
        "checkSetters": true,
        "contexts": [
          "MethodDefinition:not([key.name=/(ngAfterContentChecked|ngAfterContentInit|ngAfterViewChecked|ngAfterViewInit|ngDoBootstrap|ngDoCheck|ngOnChanges|ngOnDestroy|ngOnInit)/])", // Exclude Angular methods.
          "ClassDeclaration:not([id.name=/.*(Component|Module)/])", // Exclude Angular components.
          "TSInterfaceDeclaration", // Include interfaces.
          "TSEnumDeclaration", // Include enums.
          "ClassProperty:has(Decorator[expression.callee.name=\"Input\"])"
        ]
      }
    ]
  }
}

Tested with latest versions of all ESLint related packages. Except TypeScript, which is pinned on v4.0.5, because of Angular.

Input

export class MyComponentComponent {
    @Output()
    public changed = new EventEmitter();

    public test = 'test';

    @Input()
    public value = new EventEmitter();
}

Result

It now throws warnings on all properties, once it matches with one decorator that is @Input().

In the screenshot above you can see that it also shows an warning for the test & changed property with \@Output decorator, while only \@Input is part of the selector in the config. See the warning underline on all properties (even other types are underlined) and 3-step gradient in warning color (extension Error Lens).

It doesn't matter if I put Input or Output in the selector. It does stop throwing warning once you put for example ABC in the selector.

Notes

It also highlights another issue: the warning doesn't stop at the end of the property. Again you can see the warning underline and gradient doesn't stop at the end of the property.

This configuration is part of getting JSDoc to work for Angular project. If you are interested I can post the whole config.

@brettz9
Copy link
Collaborator

brettz9 commented Feb 1, 2021

It turns out the problem is that ESLint's selectors are not as expressive as esquery's current ones, so despite a couple tests using :has(), this is really not being used.

You might want to file an issue with ESLint about this since :has() selectors are rather useful. We are waiting on estools/esquery#112 and eslint/eslint#13639 as per eslint/eslint#13639 (comment) to get proper :has() support for TS.

In the meantime, you might be able to get away with this approach:

  1. Set ArrowFunctionExpression and FunctionExpression on require to `false.
  2. Add ArrowFunctionExpression and FunctionExpression to contexts.
  3. Use 'ClassProperty > Decorator[expression.callee.name="Input"]'. While this is finding the Decorator instead of the property, in this case, there is no practical difference since our comment detection still looks for a comment block above Decorator (just as it tries to skip above decorators) which is also what we want.

The reason for steps 1 and 2 is that our built-in require contexts have some special handling which automatically checks for these function expressions when in a ClassProperty context (regardless of decorator name). The one (probably minor) disadvantage of doing this is that the built-in function expressions, if they find the parent type to be Property, ObjectProperty, or ClassProperty, and detect that the function expression node is not also on the property's value--i.e., such that it is not a function expression within a computed property. So setting FunctionExpression in contexts would catch this:

var foo = { [function() {}]: 1 };

...whereas the built-in one ignores them in such a context. May be a bug or a feature anyways.

brettz9 added a commit to brettz9/eslint-plugin-jsdoc that referenced this issue Feb 1, 2021
…te differential behavior of selectors; gajus#662

`:has()` is a selector only available to esquery, not to the current selector engine in ESLint.
@brettz9
Copy link
Collaborator

brettz9 commented Feb 13, 2021

FWIW, ESLint 7.20.0 has now fixed support for :has() in TypeScript. Confirmed by reverting our test to use :has(). (Note though that it is better to avoid where ESLint might optimize.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants