Skip to content
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

fix: enforce specific NodeJS version #943

Merged
merged 2 commits into from
Jan 16, 2025
Merged

Conversation

plxity
Copy link
Contributor

@plxity plxity commented Dec 3, 2024

Important

Enforces Node.js version >=18.0.0 in package.json with engineStrict set to true.

  • Behavior:
    • Enforces Node.js version >=18.0.0 in package.json using engines field.
    • Sets engineStrict to true to ensure compliance with the specified Node.js version.

This description was created by Ellipsis for 0974985. It will automatically update as commits are pushed.

Copy link

vercel bot commented Dec 3, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
composio ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jan 16, 2025 1:16pm

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Looks good to me! Reviewed everything up to 0974985 in 6 seconds

More details
  • Looked at 14 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 1 drafted comments based on config settings.
1. js/package.json:86
  • Draft comment:
    Ensure that all dependencies and the codebase are compatible with Node.js 18 or higher, as specified in the engines field.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The addition of the engineStrict and engines fields in package.json is a good practice to ensure compatibility with a specific Node.js version. However, it's important to ensure that the rest of the codebase and dependencies are compatible with Node.js 18 or higher.

Workflow ID: wflow_8DP5rRviMXH2KNhR


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

Copy link

github-actions bot commented Dec 3, 2024

This comment was generated by github-actions[bot]!

JS SDK Coverage Report

📊 Coverage report for JS SDK can be found at the following URL:
https://pub-92e668239ab84bfd80ee07d61e9d2f40.r2.dev/coverage-12809678027/coverage/index.html

📁 Test report folder can be found at the following URL:
https://pub-92e668239ab84bfd80ee07d61e9d2f40.r2.dev/html-report-12809678027/html-report/report.html

},
"engineStrict": true,
"engines": {
"node": ">=18.0.0"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider adding a comment in package.json explaining why Node.js 18+ is required. This helps other developers understand the rationale behind this requirement, especially since it's a breaking change. For example:

"engines": {
  // Required for modern ES features and compatibility with key dependencies
  "node": ">=18.0.0"
}

@shreysingla11
Copy link
Collaborator

Code Review Summary

Changes Overview

  • Added explicit Node.js version requirement (>=18.0.0)
  • Added engineStrict: true to enforce version compliance
  • Location: js/package.json

Technical Assessment

Positive Aspects

  • Aligns with existing dependency requirements (e.g., @hono/node-server requires >=18.14.1)
  • Enforces consistent runtime environment across development
  • Helps prevent compatibility issues early

⚠️ Considerations

  • Breaking change for users on older Node.js versions
  • May require CI/CD pipeline updates
  • Teams need to be notified about the new requirement

Suggestions

  1. Consider adding a comment explaining the Node.js version requirement rationale
  2. Update project README to document this requirement prominently
  3. Consider adding a migration guide for users on older Node.js versions

Overall Rating: 8/10

The change is well-justified and necessary for maintaining compatibility with modern dependencies. The use of engineStrict ensures proper enforcement. Documentation could be improved to better communicate this requirement to users.

@himanshu-dixit himanshu-dixit changed the title Enforce specific NodeJS version fix: enforce specific NodeJS version Jan 16, 2025
@himanshu-dixit himanshu-dixit merged commit be2ad33 into master Jan 16, 2025
12 of 13 checks passed
@himanshu-dixit himanshu-dixit deleted the enforce-nodejs-version branch January 16, 2025 13:19
abhishekpatil4 pushed a commit that referenced this pull request Jan 20, 2025
Co-authored-by: Himanshu Dixit <hudixt@gmail.com>
abhishekpatil4 pushed a commit that referenced this pull request Jan 20, 2025
Co-authored-by: Himanshu Dixit <hudixt@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants