-
-
Notifications
You must be signed in to change notification settings - Fork 11
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 eslint peer dependency #106
Conversation
Switching to draft until we resolve #104 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Would like another review before merging.
@danielrentz can you please sign the CLA? |
done |
packages/compat/package.json
Outdated
@@ -55,6 +55,14 @@ | |||
"rollup": "^4.16.2", | |||
"typescript": "^5.4.5" | |||
}, | |||
"peerDependencies": { | |||
"@types/eslint": "^9.6.0" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now that the eslint
package itself publishes its own types (eslint/eslint#18854), @types/eslint
is no longer necessary in general. I'd assumed the plan was to turn it into a stub type definition per normal DefinitelyTyped practices. Should this be switched to "eslint"
, along with the meta lower on?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd assumed the plan was to turn it into a stub type definition per normal DefinitelyTyped practices.
We don't know normal DefiniteTyped practices. 😄 Can you open an issue somewhere for this task explaining what it is?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great, per eslint/eslint#18928 (comment) I'll mark this as needing to switch from @types/eslint
to eslint
. Thanks @snitin315!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
switched to eslint ^9.10.0
(first version with bundled typings)
@nzakas @JoshuaKGoldberg Question: Should the "optional" be removed? (Can this package be used without eslint?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think optional can stay. This package doesn't use eslint
at runtime, only at dev time to validate types. @JoshuaKGoldberg?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct place to change, but a different place in code now I believe. Thanks!
We should update |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Would like @JoshuaKGoldberg to verify before merging.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🚀
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks!
Prerequisites checklist
What is the purpose of this pull request?
This package exports TS type definitions referring to eslint types, thus it should publicly include the
@types/eslint
package.What changes did you make? (Give an overview)
In packages/compat/package.json, move "@types/eslint" to "dependencies".
Related Issues
fixes #104
fixes #105
Is there anything you'd like reviewers to focus on?