eslint-plugin-jsdoc icon indicating copy to clipboard operation
eslint-plugin-jsdoc copied to clipboard

no-undefined-types should check `@type`

Open simonseyock opened this issue 4 years ago • 6 comments
trafficstars

Motivation

Eslint complains about unused-vars if they only occur in @type annotations.

For example:

import { SomeType } from 'some/where';

const val = /** @type {SomeType} */ (otherVal);

Current behavior

no-undefined-types does not check @type annotations

Desired behavior

no-undefined-types should check @type annotations


Want to back this issue? Post a bounty on it! We accept bounties via Bountysource.

simonseyock avatar Apr 30 '21 14:04 simonseyock

Is this promoting an anti-pattern though? With "typescript" mode, one can use import() with the type (keeping it within the jsdoc), and for discovery of defined types, I think #99 would be a better approach--i.e., ourselves navigate through the project's imports (or use some config) and/or its type import()'s and auto-define typedef's and globals found therein, while not encouraging importing code that has no purpose other than static analysis.

brettz9 avatar May 10 '21 13:05 brettz9

It could be that this is considered an anti-pattern. I would argue that if I would need a variable of a certain type, that this code needs to be imported at some point in the software anyways. If I would write the same code in typescript I would need an import, too.

Also this seems to be inconsistent. If I use a type in a @param annotation it is also marked as used. So in my opinion this should be working the same for @param and @type.

It might be an option to make the whole 'marking as used' functionality optional so the user could decide which way of importing they would prefer.

simonseyock avatar May 11 '21 06:05 simonseyock

What are the benefits for using esm import over JSDoc import()? Using esm import for an unused variable leads to an unnecessary module loading at runtime (for node).

chiawendt avatar May 16 '21 17:05 chiawendt

As per https://github.com/gajus/eslint-plugin-jsdoc/issues/721#issuecomment-837929396 , I think the argument here is for consistency rather than endorsing the practice.

brettz9 avatar May 16 '21 17:05 brettz9

Yes, my main point is: Either mark all types / variables in type annotation as used or none.

I just wanted to elaborate that there might be cases in which it is possible to argue in favor of es6 imports. I think it might be nice to allow users to make this decision by themselves and make the whole 'marking as used' optional.

simonseyock avatar May 16 '21 20:05 simonseyock

Also the same should be true if you declare a variable / type in a file and only use it in an annotation.

simonseyock avatar May 16 '21 20:05 simonseyock

As mentioned just now here, I'm not sure how this got by me, but:

  1. The original issue as reported is not an issue, at least not anymore, but looking at the blame file, it seems the support for @type was preexisting the reporting of this issue.
  2. The issue might have been due either to the fact that val is indeed unused or perhaps because you had some other configuration which mimicked the problem found in #9810.

We are discussing removing the markVariableAsUnused support to its own rule, and #858 was filed to support inline {@link} tag (which indeed has not been supported). I think we can probably close this issue therefore and track all of this at #858.

brettz9 avatar May 03 '23 13:05 brettz9

there might be cases in which it is possible to argue in favor of es6 imports.

I wanted to share my use-case: The code that will be relying on this will be bundled using Rollup, which will effectively tree-shake anything that wasn't actually referenced by actual code. So this won't cause unnecessary module loading at runtime.

kraenhansen avatar May 03 '23 14:05 kraenhansen

I think we can probably close this issue therefore

I agree. I just tested and /** @type {MyClass} */ does indeed mark MyClass as used with no-undefined-types enabled.

kraenhansen avatar May 05 '23 19:05 kraenhansen