Typescript contexts in require-jsdoc

[paulius-valiunas]

The require option only allows a small subset of AST nodes, and the whitelist doesn't include anything Typescript-specific. So, in order to check for jsdoc comments at Typescript-specific AST nodes, like enums, we're advised to add the required node types to the contexts option, as per #484.

However, this doesn't seem to work. I have the following config:

    "jsdoc/require-jsdoc": [
      "error",
      {
        "publicOnly": true, // only report exports
        "require": {
          "ArrowFunctionExpression": true,
          "ClassDeclaration": true,
          "ClassExpression": true,
          "FunctionDeclaration": true,
          "FunctionExpression": true,
          "MethodDefinition": false,
        },
        "contexts": [
          "ArrowFunctionExpression",
          "ClassDeclaration",
          "ClassExpression",
          "ClassProperty",
          "FunctionDeclaration", // function
          "FunctionExpression",
          "MethodDefinition",
          "TSDeclareFunction", // function without body
          "TSEnumDeclaration",
          "TSInterfaceDeclaration",
          "TSModuleDeclaration", // namespace
          "TSTypeAliasDeclaration",
          "VariableDeclaration",
        ]
      }
    ]

Enums are still not validated. In fact, if I remove the require option, it doesn't validate anything at all, even though all these node types are still in contexts.

[brettz9]

Can you provide some sample code that highlights the issue please?

[paulius-valiunas]

Here's a code snippet:

export enum testEnum {
  A, B
}

export const storageMock = () => {
  const storage: { [key: string]: any } = {};
  return {
    setItem: (key: string, value: string) => {
      storage[key] = value || "";
    },
    getItem: (key: string) => {
      return key in storage ? storage[key] : null;
    },
    removeItem: (key: string) => {
      delete storage[key];
    },
    get length() {
      return Object.keys(storage).length;
    },
    key: (i: number) => {
      const keys = Object.keys(storage);
      return keys[i] || null;
    },
  };
};

It reports an error at storageMock only if the require option is there. If I remove the option (but leave ArrowFunctionExpression and all the other node types in contexts), the error disappears. And I just can't get an error reported at testEnum at all. AST Explorer reports it as TSEnumDeclaration, which I've added to contexts.

[paulius-valiunas]

Also, here's the full config I'm using, in case anything is missing:

module.exports = {
  "extends": [
    "plugin:jsdoc/recommended"
  ],
  "plugins": [
    "jsdoc"
  ],
  "settings": {
    "jsdoc": {
      "mode": "typescript",
      "exemptedBy": ["alpha", "internal"]
    }
  },
  "rules": {
    "jsdoc/newline-after-description": "off",
    "jsdoc/check-alignment": "off",
    "jsdoc/check-tag-names": "off",
    "jsdoc/require-param": "off",
    "jsdoc/require-param-type": "off",
    "jsdoc/require-returns": "off",
    "jsdoc/require-returns-type": "off",
    "jsdoc/require-jsdoc": [
      "error",
      {
        "publicOnly": true, // only report exports
        "require": {
          "ArrowFunctionExpression": true,
          "ClassDeclaration": true,
          "ClassExpression": true,
          "FunctionDeclaration": true,
          "FunctionExpression": true,
          "MethodDefinition": false,
        },
        "contexts": [
          "ArrowFunctionExpression",
          "ClassDeclaration",
          "ClassExpression",
          "ClassProperty",
          "FunctionDeclaration", // function
          "FunctionExpression",
          "MethodDefinition",
          "TSDeclareFunction", // function without body
          "TSEnumDeclaration",
          "TSInterfaceDeclaration",
          "TSMethodSignature",
          "TSModuleDeclaration", // namespace
          "TSTypeAliasDeclaration",
          "VariableDeclaration",
        ]
      }
    ],
  }
}

[gajus]

🎉 This issue has been resolved in version 30.5.2 🎉

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot 📦🚀

[brettz9]

I've fixed both the fact that contexts wouldn't work in place of require items and the missing support for TSEnumDeclaration.

I'm not a regular TS user, so if you find other TS AST is not working, please provide sample code for the syntax of interest, and there is a better chance we can add the support.

[paulius-valiunas]

Thanks, that was fast. I'll see if I encounter any more elements not working, but just to make it clear: anything I add to contexts now should work, as long as it's a valid AST type, right? You don't have to manually add each type, do you?

Considering there are more flavors of Javascript that add their own syntactic sugar than just Typescript, and there might come more in the future, I think the context approach is the way to go. Just for reference, here's the full list of Typescript AST types:
https://github.com/typescript-eslint/typescript-eslint/blob/master/packages/types/src/ast-node-types.ts
All the TS-prefixed types are Typescript-specific and don't exist in plain Javascript, so as you can see it almost doubles the amount of existing types, even if some of them are almost identical to normal Javascript nodes. I don't think it's productive to try and support each type individually for additional flavors of Javascript when we can have a more generic approach.

[brettz9]

In the case of public, we do currently have to manually add types, as they can require special handling.

I'm not 100% comfortable with this rule, tbh, as it is implemented with some complexity and not enough docs. I get the basic idea and can get things to work with it, but I do not have the energy to dive in to get a better feel for how we can architect it any differently for more broadly-applicable defaults, or if that is not possible (since some AST probably does require its own parsing), then to pre-populate the specific TS types that will be needed.

So you can either look into it on your own, or report some specific needs, and we can see about figuring them out one or a few at a time. I don't expect most people will be exporting every type of TS type anyways, as many of the types are inside of functions and such.

[brettz9]

If you want to help with a review, i suspect the ones with Declaration are more likely to be exportable, and thus perhaps good to begin with.

[paulius-valiunas]

Wouldn't it be safe to say that any descendant of an ExportNamedDeclaration is a public (exported) node? For example, if a class is exported, its members should also be treated as such.

The only exception I see with this is that you can have private or protected members in Typescript, which will just have an accessibility property on their AST nodes. Other than that, I think all node types should be treated the same way, unless you really want to be foolproof and point it out to the user when they're trying to whitelist something like a function argument.

Does that make sense, or am I missing something?

[brettz9]

Much of the implementation is handling checks for cjs and window which are more complicated, e.g., for code like this which discovers that test is public so reports the missing method on it:

         /**
          *
          */
          const test = module.exports = function () {

          }

          test.prototype.method = function() {}

The bulk of the logic is within src/exportParser.js (with the rule calling parse and then isExported on the result).

There is the ancestorsOnly optimization, which doesn't catch this, for example:

      function quux () {

      }
      export default quux;

Recursion is going on, but as I mentioned, you'll have to work on it on your own if you want to get into real refactoring. PRs are welcome, assuming existing tests continue to pass.