-
-
Notifications
You must be signed in to change notification settings - Fork 205
fix: remove types/eslint peer dependency; fixes #762 #763
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
base: main
Are you sure you want to change the base?
Conversation
🦋 Changeset detectedLatest commit: cea2501 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Warning Rate limit exceeded@brettz9 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 6 minutes and 41 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (4)
WalkthroughUpdated package metadata: bumped pnpm, removed Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
commit: |
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.
Important
Looks good to me! 👍
Reviewed everything up to e22a182 in 52 seconds. Click for details.
- Reviewed
29
lines of code in1
files - Skipped
0
files when reviewing. - Skipped posting
2
draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. package.json:13
- Draft comment:
Updated packageManager to [email protected]. Confirm that this upgrade is intentional and compatible with all build scripts. - Reason this comment was not posted:
Confidence changes required:0%
<= threshold50%
None
2. package.json:51
- Draft comment:
Removed '@types/eslint' from peerDependencies and peerDependenciesMeta to resolve issue #762. ESLint 9 provides its own types; ESLint 8 users can still add it manually if needed. - Reason this comment was not posted:
Confidence changes required:0%
<= threshold50%
None
Workflow ID: wflow_0fZrcb7ofi8kbV3A
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
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.
Actionable comments posted: 0
🧹 Nitpick comments (1)
package.json (1)
14-16
: Consider aligning engines.node with toolchain realityIf the maintained toolchain (ESLint 9, pnpm 10) effectively requires Node ≥18, consider bumping engines.node accordingly or documenting the split between runtime and dev requirements.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yaml
is excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (1)
package.json
(1 hunks)
🔇 Additional comments (2)
package.json (2)
51-60
: Drop @types/eslint peer dep — confirm no lingering referencesLGTM to remove; confirm locally that no references remain by searching for "@types/eslint", triple‑slash refs (/// ), or any imports from "@types/eslint" or "eslint".
13-13
: pnpm v10 bump — verify Node.js compatibility (Node 18/20/22 required)pnpm v10 requires Node.js 18/20/22; ensure package.json "engines.node" and all Node hints used by devs/CI (.nvmrc/.node-version, GitHub Actions actions/setup-node, Dockerfiles, contributor docs) target a compatible version or update "engines.node" to >=18. Automated scan returned no .nvmrc/.node-version or workflows in the repo — manually confirm those locations and CI configs are updated.
I've made a breaking change changeset just in case... |
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.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.changeset/cyan-towns-type.md (1)
5-5
: Tighten wording and add guidance + formatting.Clarify that it’s removed from peerDependencies, use code formatting, and suggest adding it to devDependencies for ESLint 8 users; reference the issue.
-Removes @types/eslint which now conflicts with ESLint 9. ESLint 8 users can still manually add @types/eslint if they so require. +Remove `@types/eslint` from `peerDependencies` to avoid conflicts with ESLint 9 (types are bundled there). +If you're on ESLint 8 and rely on typings, add `@types/eslint` to your project's `devDependencies`. See #762.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.changeset/cyan-towns-type.md
(1 hunks)
🔇 Additional comments (1)
.changeset/cyan-towns-type.md (1)
1-3
: Changeset header looks correct.Major bump is appropriate for removing a peer dependency with potential consumer impact.
7ba06bc
to
e74093f
Compare
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.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/prettier.mjs (1)
22-22
: Fix CI: import the MDX plugin via namespace (no default export).The CI error shows eslint-plugin-mdx doesn’t export default. Switch to a namespace import.
-import eslintPluginMdx from 'eslint-plugin-mdx'; +import * as eslintPluginMdx from 'eslint-plugin-mdx';
🧹 Nitpick comments (1)
test/prettier.mjs (1)
27-27
: Namespace import foreslint-mdx
is fine; ensure the parser reference is correct.When using
* as eslintMdx
, pass the actual parser object. Safer to referencedefault
when present.- parser: eslintMdx, + parser: eslintMdx.default ?? eslintMdx,Please re-run tests after the plugin import fix to confirm no parser resolution issues.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
test/prettier.mjs
(1 hunks)
🧰 Additional context used
🪛 GitHub Actions: CI
test/prettier.mjs
[error] 22-22: SyntaxError: The requested module 'eslint-plugin-mdx' does not provide an export named 'default'. Update import to use the correct named export or dynamic import. This caused the 'pnpm mocha' test run to fail with exit code 1.
Though failing Node 14 and 16, this PR drops support for those versions (rolling in #761). |
Important
Remove
@types/eslint
peer dependency and updatepnpm
version inpackage.json
.@types/eslint
frompeerDependencies
inpackage.json
.@types/eslint
frompeerDependenciesMeta
inpackage.json
.packageManager
to[email protected]
inpackage.json
.This description was created by
for e22a182. You can customize this summary. It will automatically update as commits are pushed.
Summary by CodeRabbit
Chores
Tests