Skip to content

chore: specify the correct minimum node requirements #378

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

Merged
merged 4 commits into from
Jul 17, 2025

Conversation

himanshusinghs
Copy link
Collaborator

Proposed changes

The version constraint earlier was pretty broad and did not really encapsulate the correct minimum requirements when it came to v22 and v23. This commit fixes that.

Checklist

@Copilot Copilot AI review requested due to automatic review settings July 16, 2025 14:45
@himanshusinghs himanshusinghs requested a review from a team as a code owner July 16, 2025 14:45
@himanshusinghs himanshusinghs changed the title chore: specify the correct node requirements chore: specify the correct minimum node requirements Jul 16, 2025
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR updates the Node.js version requirements in package.json to be more specific about supported versions, moving from a broad minimum requirement to explicit version ranges for Node.js 20, 22, and 23.

  • Updates the engines.node field to specify exact supported version ranges instead of a single minimum version
  • Establishes clear compatibility boundaries for Node.js v20.19.0+, v22.12.0+, and v23.0.0+

@@ -74,6 +74,6 @@
"zod": "^3.25.76"
},
"engines": {
"node": ">=20.19.0"
"node": "^20.19.0 || ^22.12.0 || >= 23.0.0"
Copy link
Preview

Copilot AI Jul 16, 2025

Choose a reason for hiding this comment

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

The version constraint syntax is inconsistent. The third range uses '>= 23.0.0' while the first two use caret ranges (^). Consider using '^23.0.0' for consistency, or if you specifically need all v23+ versions, document why this range differs from the others.

Suggested change
"node": "^20.19.0 || ^22.12.0 || >= 23.0.0"
"node": "^20.19.0 || ^22.12.0 || ^23.0.0"

Copilot uses AI. Check for mistakes.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Versions >= 23.0.0 support requiring esm by default

@coveralls
Copy link
Collaborator

coveralls commented Jul 16, 2025

Pull Request Test Coverage Report for Build 16347477296

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 3 unchanged lines in 1 file lost coverage.
  • Overall coverage decreased (-0.09%) to 77.304%

Files with Coverage Reduction New Missed Lines %
src/common/atlas/apiClient.ts 3 73.03%
Totals Coverage Status
Change from base Build 16299138293: -0.09%
Covered Lines: 2825
Relevant Lines: 3611

💛 - Coveralls

Copy link
Collaborator

@kmruiz kmruiz left a comment

Choose a reason for hiding this comment

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

Even if we decide to bundle everything into CJS, I think we need to specify the correct engine as there are libraries that we already use that complain when used in older versions from users that opened issues in this repo.

@fmenezes
Copy link
Collaborator

@himanshusinghs can you update the README.md file? There is a section where we mention minimum node version

@himanshusinghs
Copy link
Collaborator Author

Ahh yea thanks for reminding me of this - I will do that.

@himanshusinghs
Copy link
Collaborator Author

@fmenezes this is how I am thinking. Let me know if you'd prefer shorter / semver version of this. I thought a worded description might suite better.

@fmenezes
Copy link
Collaborator

Works for me

Copy link
Collaborator

@gagik gagik left a comment

Choose a reason for hiding this comment

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

ideally we want to have lower limits but yes this reflects the current reality

@gagik
Copy link
Collaborator

gagik commented Jul 17, 2025

I think this needs npm install to update package-lock?

@himanshusinghs
Copy link
Collaborator Author

himanshusinghs commented Jul 17, 2025

Weird, I did that a bunch of time and it did not update my lockfile but its true that the lockfile is still with older version.

Its updated now - I was not on correct branch (too much switching 🤦)

@himanshusinghs himanshusinghs merged commit 84e63f6 into main Jul 17, 2025
16 of 17 checks passed
@himanshusinghs himanshusinghs deleted the chore/correct-engine-requirements branch July 17, 2025 14:27
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.

5 participants