-
Notifications
You must be signed in to change notification settings - Fork 3
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
Conversation
@@ -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. |
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.
Is it perhaps worth noting here that v16 is already EOL?
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 |
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. |
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? |
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. |
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
After that test dropped the required node to v16 and pushed. |
These were massively out of date.