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

chore: add type comment to exported plugin #488

Merged

Conversation

JoshuaKGoldberg
Copy link
Contributor

@JoshuaKGoldberg JoshuaKGoldberg commented Aug 1, 2024

This will give a bit nicer of an IDE experience for folks working in editors such as VS Code that have built-in TypeScript support.

@voxpelli
Copy link
Member

voxpelli commented Aug 1, 2024

Shouldn't help as TS doesn't read JSDoc comments from node_modules?

Would need to compile it to a .d.ts?

@JoshuaKGoldberg
Copy link
Contributor Author

JoshuaKGoldberg commented Aug 1, 2024

parserOptions.projectService picks it up, it happens in the repro linked in the issue.

@voxpelli
Copy link
Member

voxpelli commented Aug 1, 2024

Even when maxNodeModuleJsDepth hasn’t been set to something other than the default 0?

Such behavior has already caused issues in ESLint as TS has an undocumented behavior of setting it when using jsconfig.json: eslint/eslint#18100 (comment)

JSDoc comments should not be relied upon

@JoshuaKGoldberg
Copy link
Contributor Author

Even when maxNodeModuleJsDepth hasn’t been set to something other than the default 0?

Yes, per the reproduction case I linked in the issue.

Agreed that .d.ts files would be better. That's more work that I don't have time for right now.

Personally, I'd rather lean towards "perfect is the enemy of good" and ship this now + hope for that eventually. There's no harm in including these type-rich JSDocs for pure .js codebases. They benefit the codebase's development itself, help pave the path for more formal TS adoption, and can also help in cases like the reproduction I linked in the issue.

@voxpelli
Copy link
Member

voxpelli commented Aug 1, 2024

Even when maxNodeModuleJsDepth hasn’t been set to something other than the default 0?

Yes, per the reproduction case I linked in the issue

So typescript eslint is not respecting the tsconfig in this regard?

I’m pro fixing #310 but against making any changes to JSDoc comments because others have decided to consume them as published types. eslint/eslint#18100 (comment) is one of many proofs that it’s a bad idea to consume JSDoc comments from third party modules

Unless this module publishes type definitions this module should be interpreted as untyped is my opinion

Copy link
Member

@voxpelli voxpelli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't object to the change itself, but I object to it being a solution to the problem at hand

@JoshuaKGoldberg
Copy link
Contributor Author

@voxpelli it's also useful for internal development and doesn't impact users using sanctioned type methods. If we ignore the reasoning around external types, do you have any remaining blockers?

@voxpelli
Copy link
Member

voxpelli commented Aug 6, 2024

@JoshuaKGoldberg no other blockers, if we change it so that this doesn't promote usage of it externally in any way then I'm okay with it

@JoshuaKGoldberg
Copy link
Contributor Author

The comment as-is is exactly what one would do for a purely internal change. There's nothing to be done to promote or not-promote it externally.

@voxpelli
Copy link
Member

voxpelli commented Aug 6, 2024

Removing the Fixes 😜

@voxpelli voxpelli changed the title feat: add type comment to exported plugin chore: add type comment to exported plugin Aug 6, 2024
@voxpelli voxpelli merged commit a3b3c05 into eslint-community:main Aug 6, 2024
7 checks passed
@JoshuaKGoldberg JoshuaKGoldberg deleted the plugin-types-comment branch August 6, 2024 17:58
@JoshuaKGoldberg
Copy link
Contributor Author

Thanks all!

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

Successfully merging this pull request may close these issues.

3 participants