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

no-undefined-types should check @type #721

Closed
simonseyock opened this issue Apr 30, 2021 · 9 comments
Closed

no-undefined-types should check @type #721

simonseyock opened this issue Apr 30, 2021 · 9 comments

Comments

@simonseyock
Copy link
Contributor

simonseyock commented Apr 30, 2021

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.

@brettz9
Copy link
Collaborator

brettz9 commented May 10, 2021

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.

@simonseyock
Copy link
Contributor Author

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.

@golopot
Copy link
Collaborator

golopot commented May 16, 2021

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).

@brettz9
Copy link
Collaborator

brettz9 commented May 16, 2021

As per #721 (comment) , I think the argument here is for consistency rather than endorsing the practice.

@simonseyock
Copy link
Contributor Author

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
Copy link
Contributor Author

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

@brettz9 brettz9 mentioned this issue May 3, 2023
2 tasks
@brettz9
Copy link
Collaborator

brettz9 commented May 3, 2023

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.

@kraenhansen
Copy link
Contributor

kraenhansen commented May 3, 2023

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
Copy link
Contributor

kraenhansen commented May 5, 2023

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.

@brettz9 brettz9 closed this as completed May 5, 2023
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