-
-
Notifications
You must be signed in to change notification settings - Fork 205
ci!: drop Node 14/16 support #761
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
Node 14 and 16 are incompatible with prettier-plugin-jsdoc-type. This commit removes them from the CI matrix and updates package.json to enforce Node >=18.18.0 and [email protected].
|
WalkthroughRemoved Node 14/16 ESLint matrix combos from CI, renamed the ESLint install step label, replaced the “Perf” step with a universal “Lint” step, and kept other CI steps unchanged. Updated engines.node to >=18.18.0 and packageManager to [email protected]. Added two NUL characters to README.md after the License section. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Dev as Push/PR
participant CI as GitHub Actions
participant Node as Node Setup
participant PNPM as pnpm
participant Tests as Test Runner
Dev->>CI: Trigger workflow
CI->>Node: Setup Node (>=18.18.0 matrix only)
CI->>PNPM: Install deps
CI->>PNPM: Install ESLint (version from matrix.eslint)
Note over CI,PNPM: Step label now references matrix.eslint
CI->>PNPM: Lint (pnpm lint)
Note over CI,PNPM: Replaces prior "Perf" step
CI->>Tests: Run tests
CI-->>Dev: Report results
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested labels
Pre-merge checks (3 passed)✅ Passed checks (3 passed)
Poem
✨ Finishing Touches🧪 Generate unit tests
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 |
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: 3
🧹 Nitpick comments (2)
.github/workflows/ci.yml (2)
27-31
: Drop commented matrix entries or justify with a noteCommented Node 14/16 include block is dead weight now that engines >=18.18.0. Either remove it or add a brief comment explaining historical context.
- # include: - # - node: 16 - # eslint: 8 - # - node: 14 - # eslint: 8 + # Older Node versions (14/16) are no longer supported (see package.json engines).
49-51
: Nit: step label uses node instead of eslint versionThe step name says “Install ESLint ${{ matrix.node }}” but it installs ESLint ${{ matrix.eslint }}. Rename for clarity.
- - name: Install ESLint ${{ matrix.node }} + - name: Install ESLint ${{ matrix.eslint }} if: ${{ matrix.eslint != 9 }} run: pnpm install -D eslint@${{ matrix.eslint }} @graphql-eslint/eslint-plugin@3 eslint-plugin-svelte@2 svelte@3 vue-eslint-parser@9
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 c1005fc in 1 minute and 30 seconds. Click for details.
- Reviewed
48
lines of code in2
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. .github/workflows/ci.yml:27
- Draft comment:
Remove commented-out code (both the 'include' matrix and 'Perf' step) if Node 14/16 support is permanently dropped. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% The comment suggests removing commented code IF Node 14/16 support is dropped. However, looking at the active Node versions (18,20,22), it appears Node 14/16 support HAS been dropped. The commented code is now just noise. The comment is speculative ("if") rather than definitive, and the condition it specifies has already been met. The code could be commented out temporarily for testing or future reference. Maybe Node 14/16 support isn't permanently dropped but just temporarily disabled. The presence of only newer Node versions (18+) in the active matrix strongly suggests this is a permanent change. Keeping commented code creates maintenance debt. Delete this comment. The condition it specifies has already been met, and it's asking for verification rather than pointing out a clear issue.
2. package.json:15
- Draft comment:
Engine version is set to '>=18.0.0' but the PR description intends Node >=18.18.0. Update the engines field accordingly. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% The comment references the PR description which we don't have access to. Without seeing the PR description, we can't verify if this suggestion is correct. This falls under the rule of needing strong evidence - we can't be sure this comment is correct without seeing the PR description. Additionally, this relates to dependency/version requirements which we're supposed to ignore. The comment might be correct if we had access to the PR description. The author might have explicitly stated they want to target Node 18.18.0. Even if the PR description does mention Node 18.18.0, our rules explicitly state we should not comment on dependency changes or assume anything about PR descriptions. We should delete this comment as it relates to dependency versioning (which we're told to ignore) and relies on PR description context we don't have.
Workflow ID: wflow_4P9wJJNfajGUtkRW
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
d52d928
to
23639ad
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: 1
🧹 Nitpick comments (1)
README.md (1)
200-200
: Switch License link to HTTPS.Minor polish; use HTTPS for opensource.org.
-[MIT](http://opensource.org/licenses/MIT) +[MIT](https://opensource.org/licenses/MIT)
Summary
Drop Node 14 and 16 from CI and enforce Node >=18.18.0.
Why
prettier-plugin-jsdoc-type
and ESLint 9+ rely on modern syntax (??=
) that isn’t available in Node <18.Changes
.github/workflows/ci.yml
to test only on Node 18, 20, and 22.package.json
:"engines": { "node": ">=18.18.0" }
Verification
pnpm lint
,pnpm prettier --check .
, andpnpm test
locally — all passed.Impact
Summary by CodeRabbit
Chores
Documentation
Important
Drop Node 14/16 support in CI and enforce Node >=18.0.0 in
package.json
..github/workflows/ci.yml
to remove Node 14 and 16 from the test matrix.package.json
to enforce Node version>=18.0.0
.This description was created by
for c1005fc. You can customize this summary. It will automatically update as commits are pushed.