-
Notifications
You must be signed in to change notification settings - Fork 13
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: hasOwn support Node.js 14 #25
Conversation
WalkthroughThe changes in this pull request involve modifications to Continuous Integration (CI) configurations and package dependencies. The Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CI
participant Node14
participant Node22
User->>CI: Push to master
CI->>Node14: Setup Node.js 14
Node14->>CI: Run tests
CI->>Node22: Setup Node.js 22
Node22->>CI: Run tests
CI->>User: CI results
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
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
🧹 Outside diff range and nitpick comments (1)
package.json (1)
Line range hint
39-41
: Critical: Node.js engine requirement conflicts with PR objective.The package.json specifies
">= 16.0.0"
as the Node.js engine requirement, but this PR's objective is to add Node.js 14 support. This needs to be updated to">= 14.0.0"
to align with the PR's goal."node": { - "engines": ">= 16.0.0" + "engines": ">= 14.0.0" },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
.github/workflows/ci.yml
(1 hunks).github/workflows/nodejs-14.yml
(1 hunks)package.json
(1 hunks)src/types/util.ts
(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- .github/workflows/nodejs-14.yml
🔇 Additional comments (2)
src/types/util.ts (1)
9-13
: LGTM! Excellent backward compatibility implementation.
The implementation correctly handles both modern and legacy Node.js versions by:
- Using Object.hasOwn when available (Node.js ≥ 16.9.0)
- Falling back to Object.prototype.hasOwnProperty.call for older versions
Let's verify the Node.js version compatibility:
.github/workflows/ci.yml (1)
16-16
: Verify Node.js 14 CI coverage.
While adding Node.js 22 is great for future-proofing, this PR's objective is to add Node.js 14 support. However:
- Node.js 14 is not included in the test matrix
- The mentioned
.github/workflows/nodejs-14.yml
file appears to be missing from the PR
Let's verify the Node.js version requirements:
@@ -64,7 +64,7 @@ | |||
"long": "^3.2.0", | |||
"semver": "^5.4.1", | |||
"ts-expect": "^1.3.0", | |||
"tshy": "^1.0.0", | |||
"tshy": "^3.0.2", |
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.
💡 Codebase verification
tshy v3.0.2 is incompatible with Node.js 14
The tshy v3.0.2 package requires Node.js 20 or >=22, which conflicts with the PR's objective to support Node.js 14. This dependency update would prevent the package from working on Node.js 14.
- Consider using an older version of tshy that supports Node.js 14
- Or remove Node.js 14 support from the PR's objectives
🔗 Analysis chain
Consider if tshy v3.0.2 supports Node.js 14.
The tshy dependency was updated to ^3.0.2. Please verify its compatibility with Node.js 14.
Let's check tshy's version requirements:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check tshy package requirements
# Expected: Should support Node.js 14
# Check tshy package.json for engine requirements
curl -s https://registry.npmjs.org/tshy/3.0.2 | jq '.engines'
Length of output: 88
[skip ci] ## [2.2.0](v2.1.0...v2.2.0) (2024-12-09) ### Features * hasOwn support Node.js 14 ([#25](#25)) ([857bdff](857bdff))
Summary by CodeRabbit
New Features
Improvements
tshy
to 3.0.2.