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 the recommended NodeJS versions. #171

Open
wants to merge 2 commits into
base: dev
Choose a base branch
from

Conversation

confluence
Copy link
Collaborator

These were massively out of date.

@confluence confluence self-assigned this Apr 29, 2024
@@ -74,7 +74,7 @@ Install CARTA controller

.. note::

Currently supported versions of NodeJS are v12, v14 and v16. In the example below we install the latest LTS version of NodeJS from the `NodeSource repo <https://github.com/nodesource/distributions>`_. Do not pass the ``--unsafe-perm`` flag to ``npm`` if using a local install.
We recommend using the `latest LTS version <https://github.com/nodejs/release#release-schedule>`_ of NodeJS. The oldest version known to work with the controller is v16. In the example below we install the latest LTS version from the `NodeSource repo <https://github.com/nodesource/distributions>`_. Do not pass the ``--unsafe-perm`` flag to ``npm`` if using a local install.
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it perhaps worth noting here that v16 is already EOL?

@daikema
Copy link
Contributor

daikema commented Apr 29, 2024

Is the idea to do this without updating anything in the code which, as mentioned in #133, seems like it can be done by modifying the package.json + .npmrc?

@confluence
Copy link
Collaborator Author

I only noticed the error in the docs. Please add the code change to this branch; it makes sense to do it at the same time.

@daikema
Copy link
Contributor

daikema commented Apr 30, 2024

Can't say that I've ever attempted to set those versions programmatically. Better to pin it at node 16 (as I think we've tested at least), or node 18 (as oldest that's not-yet-EOL)? Or does this require more conversation with other CARTA peeps first?

@confluence
Copy link
Collaborator Author

I think the minimum should be the minimum that actually works, not the minimum that is recommended. I suggest making the change, and @veggiesaurus can review it.

@daikema
Copy link
Contributor

daikema commented Apr 30, 2024

OK. So I tried adding those changes, initially with it requiring node 22, just so that it would (hopefully) fail and this is what the output of npm install looked like for me:

npm ERR! code EBADENGINE
npm ERR! engine Unsupported engine
npm ERR! engine Not compatible with your version of node/npm: carta-controller@4.1.0
npm ERR! notsup Not compatible with your version of node/npm: carta-controller@4.1.0
npm ERR! notsup Required: {"node":">=22.0.0"}
npm ERR! notsup Actual:   {"npm":"10.5.0","node":"v20.12.2"}

After that test dropped the required node to v16 and pushed.

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.

None yet

2 participants