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

[Feature Request]: Add TypeScript types #3797

Closed
2 tasks done
Tracked by #5
segevfiner opened this issue Aug 8, 2024 · 10 comments · Fixed by #3830
Closed
2 tasks done
Tracked by #5

[Feature Request]: Add TypeScript types #3797

segevfiner opened this issue Aug 8, 2024 · 10 comments · Fixed by #3830

Comments

@segevfiner
Copy link

Is there an existing issue for this?

  • I have searched the existing issues and my issue is unique
  • My issue appears in the command-line and not only in the text editor

Description Overview

It is possible to have the ESLint config be type checked using JSDoc comments and the // @ts-check annotation comment: e.g.

// @ts-check

/** @type {import("eslint").Linter.Config[]} */
export default [
    // ...
]

But this package doesn't export types ATM. It would be nice if it did export types so it works without errors using this. Most/all other plugins I used do have types.

Expected Behavior

For the package to have TypeScript types

eslint-plugin-react version

v7.35.0

eslint version

v9.8.0

node version

v18.20.4

@segevfiner segevfiner added the bug label Aug 8, 2024
@eglove
Copy link

eglove commented Aug 8, 2024

typescript-eslint provides a typed wrapper for configs.
https://typescript-eslint.io/getting-started

// @ts-check

import eslint from '@eslint/js';
import tseslint from 'typescript-eslint';

export default tseslint.config(
  eslint.configs.recommended,
  ...tseslint.configs.recommended,
);

@ljharb
Copy link
Member

ljharb commented Aug 9, 2024

I'll probably add types eventually, but in the meantime, a DT package would be the way to go.

@eric-net

This comment was marked as off-topic.

@voxpelli
Copy link
Contributor

Here's a PR that adds this through type generation based on the JSDocs: #3830

@JstnMcBrd
Copy link

@voxpelli thanks for adding this! I submitted a request for types back in #3776, so it's great to see it happen!

One issue though - I updated to v7.37.0, but TypeScript is still complaining there are no type declarations.

image

Are we sure the types are being generated + added to the package build correctly? I noticed there is no "types" field in package.json - could that be the problem? Usually packages declare their type exports like this:

"main": "index.js",
"types": "index.d.ts",

If this is a bug, should we open a new issue for this?

@voxpelli
Copy link
Contributor

I noticed there is no "types" field in package.json - could that be the problem

Oh, I forgot to add that? 🫣

Yeah, would be great to add that, can you validate that it works when that's been added?


Though I did add a test that should have validated that the types are working properly when used in a module, is that test then broken? 🤔

@JstnMcBrd
Copy link

can you validate that it works when that's been added?

I would, but I can't find the type file to point to - there's no index.d.ts. There is a lib/types.d.ts file, but it doesn't seem to define the export types, and it hasn't been modified in 6 months.

Here is what I see in my node_modules.
image

It's possible the type declarations are being excluded when publishing to npm. Or maybe I just don't know where to look.

You can see what has been published by browsing the package files here. Do you see your type files?

@voxpelli
Copy link
Contributor

Looks like the building of the types and the publishing failed: https://github.com/jsx-eslint/eslint-plugin-react/actions/runs/11062995632/job/30739408890

Yet it did publish 🤔

@ljharb
Copy link
Member

ljharb commented Oct 1, 2024

I published manually, locally. The types built fine, afaik.

Indeed, the publish workflow failed and I couldn't reproduce that locally.

If there's a bug then it'd be great to get that fixed.

@JstnMcBrd
Copy link

I'll open a separate issue for this.

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

Successfully merging a pull request may close this issue.

6 participants