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

chore: add .nvmrc file for Node.js version 22.9.0 #542

Closed

Conversation

JeevanMahesha
Copy link

PR Checklist

Please check if your PR fulfills the following requirements:


PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no API changes)
  • Build related changes
  • CI related changes
  • Documentation content changes
  • Other... Please describe:

Which package are you modifying?

  • None (This PR doesn't modify a specific package, it updates the overall project setup.)

What is the current behavior?

Currently, there is no .nvmrc file in the repository, which can lead to inconsistencies in the Node.js version used by contributors.

Closes: N/A


What is the new behavior?

The .nvmrc file has been added to specify the recommended Node.js version, ensuring that all contributors use the same version during development.


Does this PR introduce a breaking change?

  • Yes
  • No

Other information

This is an initial step to improve developer experience by introducing Node.js version management through .nvmrc.

@@ -0,0 +1 @@
22.9.0
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for this! I just noticed that we don't mention anything like this in the contributing guide, etc., but should have been added. Maybe with a different PR?

A couple of points:

  1. Please include a .node-version file with the same contents. Thanks to fragmentation between node version managers, every tool except nvm looks for a .node-version file. While nvm is stable on macOS/Linux, it often enters a problematic state in Windows, so many people have switched to other alternatives.
  2. I can't recall where I remember this from, but .nvmrc (and .node-version) should only include the major version to allow for easy upgrades through minor versions.
Suggested change
22.9.0
22

Copy link
Author

Choose a reason for hiding this comment

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

@ajitzero Thank you for the feedback!

I’d like to address the points you raised:

  1. Regarding .node-version: We already have a .node-version file for Windows users, and this PR extends support for macOS and Linux users by introducing .nvmrc. This ensures broader compatibility across platforms while keeping the workflow seamless for contributors.

  2. Specific Version vs. Major Version:
    I understand your suggestion to use only the major version for flexibility and ease of upgrades. However, for open-source projects, using a specific version often ensures consistency across development, CI pipelines, and production environments. This avoids potential issues such as:

    • Non-reproducible bugs if contributors or CI pipelines use different patch or minor versions.
    • Unexpected regressions or deprecations introduced in minor or patch updates, which can disrupt the workflow.

While I see the value in using the major version for personal or fast-moving projects, I believe specifying an exact version is more suitable for open-source projects where consistency and reproducibility are critical.

Let me know if you'd like to discuss further or if you'd prefer a separate PR for .node-version adjustments.

Thanks again for your thoughtful review!

Copy link
Contributor

@ajitzero ajitzero Dec 26, 2024

Choose a reason for hiding this comment

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

  1. We already have a .node-version file

Whoops! I missed that.

This avoids potential issues such as

So again, I'm really not sure what/who I'm recalling when I say we should only save the major version, so feel free to ignore that part.

The minor versions in Node have been very stable over time and ideally shouldn't introduce any breaking changes. I think its fine if we leave it in though.


I'm now considering that nvm users should just symlink .node-version to .nvmrc instead us maintaining two files for the same thing. A lot of better alternatives exist now, which all support .node-version.

While the maintainer behind nvm refuses to support a standard format (due to lack of personal bandwidth), I guess this project probably doesn't use nvm either, since we have a .node-version file in here.

I personally use fnm, but maybe others could chime in? Instead, we could update the contributing guide to mention our preferred node version management tool. cc/ @goetzrobin @ashley-hunter @thatsamsonkid

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, the issue seems to be there isn't really a standard, I used NVM for a while, but recently a macOS update made it significantly slower. As a result I switched to Volta, however Volta uses a different convention for specifying the node version again, so adding more and more configs doesn't really seem like a great solution.

We do already define the required version in the engines field in package.json:

"node": "20.x || 21.x || 22.x",

This should throw an error when trying to run PNPM install if you are using an unsupported version, which would result in the user having to use the correct node version in whatever node version manager they use - if any.

So perhaps that is enough? Maybe just add a section to the contributing guide specifying the required version?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I agree with @ashley-hunter here. Ultimately it's up to the developer to decide which tool they want to use to get the right node version to run the project. I think we can add a section to the contributing guide and if you'd like to describe your setup with nvm

Copy link

Choose a reason for hiding this comment

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

Let CI/CD checks have final say, easy

@JeevanMahesha
Copy link
Author

After review and considering the current project setup, I’ve decided to close this PR. The existing configuration, which specifies the required Node.js versions in the engines field of package.json, already effectively handles the use case this PR aimed to address.

Thank you.

@JeevanMahesha JeevanMahesha deleted the feature/add-nvmrc branch January 4, 2025 14:21
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