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

overloaded TypeScript functions accept jsdoc only on implementation #666

Closed
midgleyc opened this issue Jan 6, 2021 · 12 comments
Closed

Comments

@midgleyc
Copy link

midgleyc commented Jan 6, 2021

Expected behavior

Given an overloaded typescript function, a JSDoc at the top of the series of functions should count for all of them.

Actual behavior

The JSDoc comment is required to be on the implementation function.

A comment only on the implementation function is passed through to the .js file, but not to the .d.ts file, and isn't picked up by e.g. VSCode as describing the function.

ESLint Config

{
  "parser": "@typescript-eslint/parser",
  "plugins": [
    "@typescript-eslint",
    "jsdoc"
  ],  
  "parserOptions": {
    "ecmaVersion": 2020
  },  
  "rules": {
    "jsdoc/require-jsdoc": 1
  }
}

ESLint sample

/**
 * Adds two things
 */
export function add(a: number, b: number): number
export function add(a: string, b: string): string
export function add(a: any, b: any): any {
  return a + b
}

Should not error, but does.

Environment

  • Node version: v14.15.4
  • ESLint version: 7.17.0
  • eslint-plugin-jsdoc version: 30.7.13
@brettz9
Copy link
Collaborator

brettz9 commented Jan 6, 2021

Within the examples for the jsdoc/require-jsdoc rule, there is an example showing how this can be done by using the following option:

        {
          contexts: [
            'TSDeclareFunction:not(TSDeclareFunction + TSDeclareFunction)',
            'FunctionDeclaration:not(TSDeclareFunction + FunctionDeclaration)',
          ],
          require: {
            FunctionDeclaration: false,
          },
        }

This disables the normal required reporting about all plain function declarations (FunctionDeclaration: false), but then reinforces it for FunctionDeclaration which is not preceded by a TypeScript (overloaded signature) declaration as well as for TypeScript (overloaded signature) declarations not preceded by another overloaded declaration.

I'm not sure we should change the default here because, besides adding (TS) parser-specific complexity to the code, I'm not sure every project necessarily wants things to work this way.

@midgleyc
Copy link
Author

midgleyc commented Jan 7, 2021

Where are the examples located? I didn't see them in the README.

I agree that if this can be configured to work with TypeScript correctly, there's no need to change the default.

Unfortunately, that configuration didn't work for me. For the file:

/**
 * Adds two things
 */
export function add(a: number, b: number): number
export function add(a: string, b: string): string
export function add(a: any, b: any): any {
        return a + b
}

and eslint configuration

{
  "parser": "@typescript-eslint/parser",
  "plugins": [
    "@typescript-eslint",
    "jsdoc"
  ],  
  "parserOptions": {
    "ecmaVersion": 2020
  },  
  "rules": {
    "jsdoc/require-jsdoc": [
      "warn",
      {   
        "contexts": [
          "TSDeclareFunction:not(TSDeclareFunction + TSDeclareFunction)",
          "FunctionDeclaration:not(TSDeclareFunction + FunctionDeclaration)"
        ],  
        "require": {
          "FunctionDeclaration": false
        }   
      }   
    ]   
  }
}

when linting I get

  5:8  warning  Missing JSDoc comment  jsdoc/require-jsdoc
  6:8  warning  Missing JSDoc comment  jsdoc/require-jsdoc

It looks to me like the overloads without bodies are being interpreted as FunctionDeclaration instead of TSDeclareFunction.

@brettz9
Copy link
Collaborator

brettz9 commented Jan 7, 2021

For the example, search for TSDeclareFunction:not(TSDeclareFunction + TSDeclareFunction). It is there.

As far as the error, my apologies, In this case, if you check https://astexplorer.net/ , you'll see that you need ExportNamedDeclaration instead of FunctionDeclaration, so try this instead:

        {
          contexts: [
            'TSDeclareFunction:not(TSDeclareFunction + TSDeclareFunction)',
            'ExportNamedDeclaration:not(TSDeclareFunction + ExportNamedDeclaration',
          ],
          require: {
            FunctionDeclaration: false,
          },
        }

@midgleyc
Copy link
Author

midgleyc commented Jan 7, 2021

That doesn't work either (I fixed the missing bracket). Now it warns on 5:1, 5:8 and 6:1.

Looking at TypeScript AST Viewer I think what I need is to warn on a FunctionDeclaration which is preceded by a FunctionDeclaration with a block, and the very first FunctionDeclaration of the file.

Can I delve into an AST node this way? Thanks for your help here.

@midgleyc
Copy link
Author

midgleyc commented Jan 7, 2021

Actually, forget that, the example immediately after the example you gave works by the look of it.

{
  "contexts": [ 
    "ExportNamedDeclaration[declaration.type=\"TSDeclareFunction\"]:not(ExportNamedDeclaration[declaration.type=\"TSDeclareFunction\"] + ExportNamedDeclaration[declaration.type=\"TSDeclareFunction\"])",
    "ExportNamedDeclaration[declaration.type=\"FunctionDeclaration\"]:not(ExportNamedDeclaration[declaration.type=\"TSDeclareFunction\"] + ExportNamedDeclaration[declaration.type=\"FunctionDeclaration\"])"
  ],
  "require": { 
    "FunctionDeclaration": false
  } 
}

@brettz9
Copy link
Collaborator

brettz9 commented Jan 9, 2021

Did that work for ya? I'll close in assuming it was ok if I don't hear back.

@midgleyc
Copy link
Author

midgleyc commented Jan 9, 2021

Yeah, that one looks to be good, thanks.

@midgleyc midgleyc closed this as completed Jan 9, 2021
@dartess
Copy link

dartess commented Jan 19, 2021

@midgleyc @brettz9 hi guys

Thanks for this issue, i met this problem

Last solution did not work for me because I have an export written before the function. unfortunately, I do not quite understand how I can adapt the rule setting so that it works for overloads with export. if you have the opportunity, please help.

My code:

/**
 * Calculate a 32 bit FNV-1a hash
 * Found here: https://gist.github.com/vaiorabbit/5657561
 * Ref.: http://isthe.com/chongo/tech/comp/fnv/
 *
 * @param str the input value
 * @param [asString=false] set to true to return the hash value as
 *     8-digit hex string instead of an integer
 * @param [seed] optionally pass the hash of the previous chunk
 *
 * @returns хэк
 */
export function hashFnv32a(str: string, asString: true, seed?: number): string;
export function hashFnv32a(str: string, asString?: false, seed?: number): number;
export function hashFnv32a(str: string, asString?: boolean, seed?: number): string | number { ... }

with #666 (comment) I get warnings

@brettz9
Copy link
Collaborator

brettz9 commented Jan 19, 2021

Did you add

require: {
        FunctionDeclaration: false,
      }

...in addition to contexts. In my testing I get no require-jsdoc warnings. I suggest filing a new issue if you are still having problems, as this seems to work for me (removing your ending ... of course).

@dartess
Copy link

dartess commented Jan 19, 2021

I removed this and get the same result

        'jsdoc/require-jsdoc': [
            'warn',
            {
                contexts: [
                    'TSDeclareFunction:not(TSDeclareFunction + TSDeclareFunction)',
                    'FunctionDeclaration:not(TSDeclareFunction + FunctionDeclaration)'
                ],
            },
        ],

image

I'm not sure if I want to open a separate issue for this, sorry. I think this is relevant to the current issue. For myself, I found a workround: I removed exports from function declarations and added named exports later.

@brettz9
Copy link
Collaborator

brettz9 commented Jan 19, 2021

I was just asking whether you had added:

require: {
  FunctionDeclaration: false,
}

..as it should be there if using the contexts. I will need a minimal test repo if you are still experiencing the issue, as again, I am not simulating in our test environment with:

{
    code: `
/**
 * Calculate a 32 bit FNV-1a hash
 * Found here: https://gist.github.com/vaiorabbit/5657561
 * Ref.: http://isthe.com/chongo/tech/comp/fnv/
 *
 * @param str the input value
 * @param [asString=false] set to true to return the hash value as
 *     8-digit hex string instead of an integer
 * @param [seed] optionally pass the hash of the previous chunk
 *
 * @returns хэк
 */
export function hashFnv32a(str: string, asString: true, seed?: number): string;
export function hashFnv32a(str: string, asString?: false, seed?: number): number;
export function hashFnv32a(str: string, asString?: boolean, seed?: number): string | number { }`,
    options: [{
      contexts: [
        // eslint-disable-next-line max-len
        'ExportNamedDeclaration[declaration.type="TSDeclareFunction"]:not(ExportNamedDeclaration[declaration.type="TSDeclareFunction"] + ExportNamedDeclaration[declaration.type="TSDeclareFunction"])',
        // eslint-disable-next-line max-len
        'ExportNamedDeclaration[declaration.type="FunctionDeclaration"]:not(ExportNamedDeclaration[declaration.type="TSDeclareFunction"] + ExportNamedDeclaration[declaration.type="FunctionDeclaration"])',
      ],
      require: {
        FunctionDeclaration: false,
      },
    }],
    parser: require.resolve('@typescript-eslint/parser'),
  },

@GerkinDev
Copy link

GerkinDev commented Jul 29, 2021

For classes method overloads, best I could get was the following context mimicking midgleyc's answer above:

{
	contexts: [
		'ExportNamedDeclaration[declaration.type="TSDeclareFunction"]:not(ExportNamedDeclaration[declaration.type="TSDeclareFunction"] + ExportNamedDeclaration[declaration.type="TSDeclareFunction"])',
		'ExportNamedDeclaration[declaration.type="FunctionDeclaration"]:not(ExportNamedDeclaration[declaration.type="TSDeclareFunction"] + ExportNamedDeclaration[declaration.type="FunctionDeclaration"])',
		'MethodDefinition[value.type="TSEmptyBodyFunctionExpression"]:not(MethodDefinition[value.type="TSEmptyBodyFunctionExpression"] + MethodDefinition[value.type="TSEmptyBodyFunctionExpression"])',
		'MethodDefinition[value.type="FunctionExpression"]:not(MethodDefinition[value.type="TSEmptyBodyFunctionExpression"] + MethodDefinition[value.type="FunctionExpression"])',
	],
	require: {
		FunctionDeclaration: false,
	},
}

Note that you can omit constructor & get/set with following contexts:

'MethodDefinition[kind="method"][value.type="TSEmptyBodyFunctionExpression"]:not(MethodDefinition[value.type="TSEmptyBodyFunctionExpression"] + MethodDefinition[value.type="TSEmptyBodyFunctionExpression"])',
'MethodDefinition[kind="method"][value.type="FunctionExpression"]:not(MethodDefinition[value.type="TSEmptyBodyFunctionExpression"] + MethodDefinition[value.type="FunctionExpression"])',

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

No branches or pull requests

4 participants