feat: add missing type definitions #246
Conversation
aryaemami59
force-pushed
the
type-definitions
branch
2 times, most recently
from
5826543
to
d8f9980
Compare
configs.js
Outdated
There was a problem hiding this comment.
We should revert this file as well
There was a problem hiding this comment.
The reason I made those changes is because it would otherwise cause issues when with named exports in ESM files, here is a before and after:
Before:
After:
| "typesVersions": { | ||
| "*": { | ||
| "configs": [ | ||
| "./types/configs.d.ts" | ||
| ] | ||
| } |
There was a problem hiding this comment.
We already have configs types defined, so I don't think it's necessary to declare them again in typesVersions?
There was a problem hiding this comment.
Without it Node10 module resolution will fail when importing from @eslint-community/eslint-plugin-eslint-comments/configs.
package.json
Outdated
| "@babel/core": "^7.22.9", | ||
| "@babel/eslint-parser": "^7.22.9", | ||
| "@eslint-community/eslint-plugin-mysticatea": "^15.5.1", | ||
| "@types/eslint": "^9.6.1", |
There was a problem hiding this comment.
Maybe it's best to shadow the eslint version here? (^8)
There was a problem hiding this comment.
We could update version of eslint instead since eslint now provides its own type definitions so no need to use @types/eslint. Thoughts?
aryaemami59
force-pushed
the
type-definitions
branch
from
bea3155
to
479ac3d
Compare
aryaemami59
force-pushed
the
type-definitions
branch
from
479ac3d
to
e05e75f
Compare
aryaemami59
force-pushed
the
type-definitions
branch
from
e05e75f
to
e603aa5
Compare
|
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. |
|
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 :) |
aryaemami59
force-pushed
the
type-definitions
branch
from
47d2440
to
8683642
Compare
| import configs = require("@eslint-community/eslint-plugin-eslint-comments/configs") | ||
| import expectTypeModule = require("expect-type") | ||
|
|
||
| import type { Linter } from "eslint" | ||
|
|
||
| import expectTypeOf = expectTypeModule.expectTypeOf |
There was a problem hiding this comment.
Should most of this not be const instead of import?
| 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 😅
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Oh, neat! Thanks for the explanation :)
Edit: I can't seem to mark this as resolved 👀
There was a problem hiding this comment.
@scagood It might be good to fully migrate to TypeScript so the types and runtime code don't get out of sync. Thoughts?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
This PR: