chore(node): support Node >=18; test 18/20/22/24; dev on Node 24 types #276
Conversation
…mp to ^24.x; tsup target node18; CI test Node 18/20/22/24 Co-Authored-By: Kevin Elliott <kevin@welikeinc.com>
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
Bug Report
Comments? Email us. |
WalkthroughUpdates CI Node.js matrix to explicit versions (18, 20, 22, 24). Adds Node engine constraint (>=18), moves and bumps Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (1)
🧰 Additional context used🧬 Code Graph Analysis (1)lib/plugins/Label_H1_OHMA.ts (1)
🔇 Additional comments (1)
✨ 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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
package.json (1)
42-42: Action: Verify that published typings don't leak Node types (Buffer / NodeJS)Quick results from a repo scan — please verify built declarations:
Findings:
- Internal Buffer usage: lib/utils/miam.ts (lines 141, 153, 180, 225, 316, 330, 416, 480, 486).
- Buffer.from used: lib/plugins/Label_H1_OHMA.ts:25.
- CLI uses process.argv: lib/bin/acars-decoder.ts (lines 7–8) — CLI/runtime usage only.
- No exported declarations that mention Buffer/NodeJS on the same line were found.
- I did not find committed .d.ts files to inspect (can't confirm what your built dist/*.d.ts will contain).
Recommended next steps:
- Inspect the built declaration files (dist/*.d.ts). If any declaration references Buffer/NodeJS or contains /// , consumers will need @types/node installed.
- If declarations do leak Node types, either keep @types/node in dependencies, add it as a peerDependency, or document the requirement in README/tsconfig guidance.
- Optionally add a CI check that scans generated .d.ts for Node-type references.
Context snippet:
"@types/node": "^24.2.1",.github/workflows/yarn-test.yml (2)
19-20: Consider adding fail-fast: false so all Node versions complete even if one fails.
This helps you see breakage patterns across versions in one run.Apply within the strategy block:
strategy: + fail-fast: false matrix: # build against all active LTS and current stable node node-version: [18.x, 20.x, 22.x, 24.x]Additionally, add a build + smoke test of the published entrypoints to validate tsup output on each Node version:
# after "Run tests" - name: Build dist run: npm run build - name: Smoke test CJS import run: node -e "require('./dist')" - name: Smoke test ESM import run: node --input-type=module -e "import('./dist/index.mjs').then(()=>{}).catch(err=>{console.error(err); process.exit(1);})"
19-20: Workflow name suggests Yarn, but CI uses npm; package.json specifies packageManager: yarn@4.5.1.
If you intend to standardize on npm (package-lock.json is present), consider updating package.json’s packageManager field and renaming the workflow file. If you intend to standardize on Yarn v4, switch CI steps to Yarn (e.g., enable corepack, useyarn install --immutable, andyarn test). Keeping both can drift.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (3)
.github/workflows/yarn-test.yml(1 hunks)package.json(2 hunks)tsup.config.ts(1 hunks)
🔇 Additional comments (4)
package.json (1)
19-21: Engines field set to Node >=18 is appropriate and aligned with CI and build target.
This cleanly communicates support policy and matches the tsup target and CI matrix. LGTM.tsup.config.ts (2)
5-6: No functional change to format/dts — dual CJS/ESM and type generation remain.
Good to keep both entry formats and dts enabled.
10-10: Targeting "node18" is the right call to ensure JS feature downleveling.
This matches the engines constraint and CI. Just ensure tests catch accidental usage of Node 24-only runtime APIs (types are 24.x in dev)..github/workflows/yarn-test.yml (1)
19-20: Explicit LTS + current stable versions in the matrix is a solid improvement.
Replacing “latest” with pinned majors avoids surprises and keeps runs deterministic. LGTM.
…port policy Co-Authored-By: Kevin Elliott <kevin@welikeinc.com>
Bug ReportNo bugs found. Comments? Email us. |
kevinelliott
added
documentation
daviesgeek
left a comment
daviesgeek
left a comment
There was a problem hiding this comment.
This looks good, didn't pull it down locally but if ci runs, is good.
Thanks for the CR! |
chore(node): support Node >=18; test 18/20/22/24; dev on Node 24 types
Summary
Updates the Node.js support matrix to explicitly support Node >=18 while allowing development with the latest stable Node 24 types. This addresses the dependabot PR that wanted to bump
@types/nodeto v24 by implementing a compatibility strategy that doesn't force Node 24 types onto library consumers.Key Changes:
engines.node: ">=18"to explicitly communicate runtime support@types/nodefromdependencies→devDependenciesand bumped^22.13.10→^24.2.1tsuptarget to"node18"to ensure compiled output stays compatible with Node 18+[18.x, 20.x, latest]→[18.x, 20.x, 22.x, 24.x]for deterministic testingThis preserves broad runtime compatibility (Node 18+) while allowing the development team to use modern Node 24 typings for better DX.
Review & Testing Checklist for Human
dist/files to ensure no Node 19+ syntax is emittedDiagram
%%{ init : { "theme" : "default" }}%% graph TB PJ["package.json<br/>engines + deps"]:::major-edit TC["tsup.config.ts<br/>target: node18"]:::major-edit CI[".github/workflows/<br/>yarn-test.yml<br/>matrix: 18/20/22/24"]:::major-edit LIB["lib/**/*.ts<br/>Source Code"]:::context DIST["dist/**<br/>Built Output"]:::context PJ -->|"declares runtime<br/>support >=18"| LIB TC -->|"compiles for<br/>Node 18+"| DIST CI -->|"validates on<br/>4 Node versions"| DIST LIB -->|"built by tsup"| DIST subgraph Legend L1[Major Edit]:::major-edit L2[Minor Edit]:::minor-edit L3[Context/No Edit]:::context end classDef major-edit fill:#90EE90 classDef minor-edit fill:#87CEEB classDef context fill:#FFFFFFNotes
npm install && npm run build && npm testSummary by CodeRabbit
Tests
Chores
Documentation