gajus / eslint-plugin-jsdoc

JSDoc specific linting rules for ESLint.
Other
1.09k stars 157 forks source link

Ignore certain sections of Vue components #1152

Closed duncan-oxd closed 1 year ago

duncan-oxd commented 1 year ago

Motivation

With Vue single file components (.vue files), this plugin will flag things like lifecycle hooks (created(), mounted(), ...), computed getters, and watchers as methods in need of JSDocs.

Using the options API, a component might look like this, with a lot of built-in stuff written as methods in a single object:

export default {
  created() {
    this.getData();
  },

  beforeUpdate() {...},

  watch: {
    someValue(val) {...}
  },

  computed: {
    loaded() {...},
    selection() {...}
  },

  methods: {
    getData(id) {...}
  }
};

Some of those might be more appropriate for other kinds of comments, as JSDocs might not add much value for things like lifecycle hooks. Edit: and that's not to mention the data() { ... } block that's present in almost every component!

Current behavior

In the example above, you'd get errors or warnings from the jsdoc/require-jsdoc rule about all 6 methods in the code: created, beforeUpdate, someValue, loaded, selection, and getData.

Desired behavior

In the eslint configuration, it would be good to have a way to ignore all but the methods defined in methods. The only warning/error you'd get would be about getData in the code example above.

Alternatives considered

Leaving the rule enabled globally, but adding // eslint-disable-line ... comments in all of these places in every file seems cumbersome. The best compromise right now seems to be setting jsdoc/require-jsdoc to off and using the other rules to aid in auto-fixing formatting errors.

I tried adding an override for *.vue files in my Eslint config and specifying the contexts according to the vue eslint parser, but I couldn't get it to work.

brettz9 commented 1 year ago

Can you try the contexts you desire in a non-Vue setting and see if that matches first, and let us know what you tried?

duncan-oxd commented 1 year ago

I couldn't figure out how to specify options for the contexts, but with this array:

        "contexts": [
          "FunctionDeclaration",
          "MethodDefinition",
          "FunctionExpression"
        ]

It lints the method types I want. If I remove FunctionExpression, for example, it stops linting the methods I defined with that syntax. So, I know that much is working, but I don't know how to specify where in the code as the context.

brettz9 commented 1 year ago

Try contexts such as this:

contexts: [
  'ExportDefaultDeclaration > ObjectExpression > Property[key.name!=/^(created|beforeUpdate)$/] > FunctionExpression',
  'ExportDefaultDeclaration > ObjectExpression > Property[key.name!=/^(watch|computed|methods)$/] > ObjectExpression > Property > FunctionExpression',
],
duncan-oxd commented 1 year ago

Thank you, @brettz9 ! That syntax is what I was looking for. I changed the first line to ignore all of the lifecycle hooks, and changed the second line to exclude watch and computed, but to not exclude methods:

"contexts": [
  "FunctionDeclaration",
  "MethodDefinition",
  "ExportDefaultDeclaration > ObjectExpression > Property[key.name!=/^(beforeCreate|created|beforeMount|mounted|beforeUpdate|updated|activated|deactivated|beforeDestroy|destroyed|errorCaptured|beforeRouteEnter|beforeRouteUpdate|beforeRouteLeave|data)$/] > FunctionExpression",
  "ExportDefaultDeclaration > ObjectExpression > Property[key.name!=/^(watch|computed)$/] > ObjectExpression > Property > FunctionExpression"
]

I think I'm on the right track now 😃 🙌

I had no idea how to use this AST selector syntax. Could you tell me where I'd find documentation to learn more about that?

It seems to work the way I want now, but I'm not sure I'm effectively mixing the requires global rule with this contexts rule in the overrides.

My global rule config:

"jsdoc/require-jsdoc": [
  "warn",
  {
    "require": {
      "ArrowFunctionExpression": false,
      "ClassDeclaration": false,
      "ClassExpression": false,
      "FunctionDeclaration": true,
      "FunctionExpression": true,
      "MethodDefinition": true
    }
  }
],

...and my override:

{
  "files": [
    "*.vue",
    "**/mixins/**/*.js"
  ],
  "parser": "vue-eslint-parser",
  "rules": {
    "jsdoc/require-jsdoc": [
      "error",
      {
        "contexts": [
          "FunctionDeclaration",
          "MethodDefinition",
          "ExportDefaultDeclaration > ObjectExpression > Property[key.name!=/^(beforeCreate|created|beforeMount|mounted|beforeUpdate|updated|activated|deactivated|beforeDestroy|destroyed|errorCaptured|beforeRouteEnter|beforeRouteUpdate|beforeRouteLeave|data)$/] > FunctionExpression",
          "ExportDefaultDeclaration > ObjectExpression > Property[key.name!=/^(watch|computed)$/] > ObjectExpression > Property > FunctionExpression"
        ]
      }
    ]
  }
}

Does that look correct? Or is some of that redundant/conflicting?

As far as my "feature request" goes, I guess it turns out this capability was already built in. Maybe a shorthand option like vueMethods: true in the rule config would be convenient, but we can certainly use contexts instead.

brettz9 commented 1 year ago

I had no idea how to use this AST selector syntax. Could you tell me where I'd find documentation to learn more about that?

The documentation is not very detailed, but you can see esquery which is the basis of the querying. The test cases might clarify if the README doesn't.

It seems to work the way I want now, but I'm not sure I'm effectively mixing the requires global rule with this contexts rule in the overrides.

It really depends on what you want to trigger. What you did should work except that if you rely on the default, you don't need to add it to contexts. The default of require is for FunctionDeclaration to be true, so you'll have to disable that if you don't want it triggering (e.g., if you decide to only allow more precise FunctionDeclaration's).

Using https://astexplorer.net is very helpful in learning the AST.

As far as my "feature request" goes, I guess it turns out this capability was already built in. Maybe a shorthand option like vueMethods: true in the rule config would be convenient, but we can certainly use contexts instead.

Yeah, I'm not keen for us to get framework-dependent, especially since projects may differ in what to allow.

brettz9 commented 1 year ago

Closing as the above should clarify, but feel free to comment further as needed.