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

feat: add missing type definitions #246

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

aryaemami59
Copy link

This PR:

@aryaemami59 aryaemami59 force-pushed the type-definitions branch 2 times, most recently from 5826543 to d8f9980 Compare October 26, 2024 21:56
@aryaemami59 aryaemami59 marked this pull request as ready for review October 26, 2024 22:11
Copy link

@scagood scagood left a comment

Choose a reason for hiding this comment

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

Generally looks good to me 🙇‍♂️

@aryaemami59 aryaemami59 force-pushed the type-definitions branch 2 times, most recently from e05e75f to e603aa5 Compare November 13, 2024 11:30
@aryaemami59
Copy link
Author

I'm thinking about adding some type tests to instill some more confidence, let me know if that's something you would be interested in.

@scagood
Copy link

scagood commented Nov 17, 2024

That very much would be!

We have an issue in a different eslint-community repo to do exactly that (eslint-community/eslint-plugin-n#212). So I would (possibly) look to 'borrow' the prior art if you do end up adding some tests :)

Comment on lines +1 to +6
import configs = require("@eslint-community/eslint-plugin-eslint-comments/configs")
import expectTypeModule = require("expect-type")

import type { Linter } from "eslint"

import expectTypeOf = expectTypeModule.expectTypeOf
Copy link

Choose a reason for hiding this comment

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

Should most of this not be const instead of import?

Suggested change
import configs = require("@eslint-community/eslint-plugin-eslint-comments/configs")
import expectTypeModule = require("expect-type")
import type { Linter } from "eslint"
import expectTypeOf = expectTypeModule.expectTypeOf
const configs = require("@eslint-community/eslint-plugin-eslint-comments/configs")
const { expectTypeOf } = require("expect-type")
import type { Linter } from "eslint"

I have never seen this syntax before, so do correct me if I am wrong 😅

Copy link
Author

Choose a reason for hiding this comment

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

In this file I'm testing the types to make sure they work with TypeScript's CJS syntax. So something like const configs = require("@eslint-community/eslint-plugin-eslint-comments/configs") becomes import configs = require("@eslint-community/eslint-plugin-eslint-comments/configs"). It's to test ESM/CJS Interoperability on a type level.

Reference

Copy link

@scagood scagood Nov 20, 2024

Choose a reason for hiding this comment

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

Oh, neat! Thanks for the explanation :)


Edit: I can't seem to mark this as resolved 👀

Copy link
Author

@aryaemami59 aryaemami59 Nov 20, 2024

Choose a reason for hiding this comment

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

@scagood It might be good to fully migrate to TypeScript so the types and runtime code don't get out of sync. Thoughts?

Copy link

Choose a reason for hiding this comment

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

Here is the previous discussion in the eslint-plugin-n repo, I am okay with either tbh
eslint-community/eslint-plugin-n#371


That being said, that is more a question for @eslint-community/eslint-plugin-eslint-comments.

Copy link
Author

Choose a reason for hiding this comment

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

ah I see, honestly I would be happy to do it, I've done a lot of TS migrations and I could definitely do this one too. We can bundle with tsup which would be even easier. Do you want me to put together a quick POC so you can kind of see what it will look like?

@carlocorradini
Copy link

Any update? Thanks 🙌

@aryaemami59
Copy link
Author

Any update? Thanks 🙌

Sorry about that, I kind of got busy and neglected this, I did mention a while back that I think it's better if we do a full TS migration so the types remain accurate and stay in sync with the runtime code. That offer still stands, I would love to do that if you guys are okay with it.

@carlocorradini
Copy link

@aryaemami59 Awesome thanks 🙂‍↕️🥳

@aryaemami59 aryaemami59 force-pushed the type-definitions branch 2 times, most recently from 5ef2840 to 1e4b665 Compare March 2, 2025 13:23
@tomer-dev
Copy link

Looks great! Thank you 🚀

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.

missing TypeScript declaration file
7 participants