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

JS-259 Do not warn against active Node.js versions #4842

Merged
merged 2 commits into from
Sep 20, 2024

Conversation

yassin-kammoun-sonarsource
Copy link
Contributor

No description provided.

@yassin-kammoun-sonarsource
Copy link
Contributor Author

The ticket mentions testing the CI against all active Node.js versions.
This will really slow down our pull requests.
Is it really something we want to do?

@@ -54,8 +53,7 @@ public class NodeDeprecationWarning {
*/
static final Version MIN_SUPPORTED_NODE_VERSION = Version.create(18, 17, 0);
static final int MIN_RECOMMENDED_NODE_VERSION = 18;
static final List<String> RECOMMENDED_NODE_VERSIONS = List.of("^18.18.0", "^20.9.0");
static final List<Integer> ALL_RECOMMENDED_NODE_VERSIONS = Arrays.asList(18, 20, 21);
static final List<String> RECOMMENDED_NODE_VERSIONS = List.of("^18.18.0", "^20.9.0", "^22.9.0");
Copy link
Contributor Author

Choose a reason for hiding this comment

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

How were 18.18.0 and 20.9.0 chosen?
Should they be updated according to the current latest versions?

Copy link
Contributor

Choose a reason for hiding this comment

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

I believe the minor versions follow ESLint package.json engine field https://github.com/eslint/eslint/blob/main/package.json#L204 , I wouldn't update unless we have a reason to

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

@yassin-kammoun-sonarsource
Copy link
Contributor Author

yassin-kammoun-sonarsource commented Sep 20, 2024

The following dates have expired for a while now:

static final Map<Integer, String> REMOVAL_DATE = Map.ofEntries(
entry(16, "Jan 31th, 2024"),
entry(17, "Jan 31th, 2024")
);

However, we still log the following message:

String msg = String.format(
"Using Node.js version %d to execute analysis is deprecated and will stop being supported no earlier than %s." +
" Please upgrade to a newer LTS version of Node.js %s",
actualNodeVersion,
REMOVAL_DATE.get(actualNodeVersion),
RECOMMENDED_NODE_VERSIONS

Should we officially acknowledge and announce that we don't support Node.js 16 and Node.js 17?

@saberduck
Copy link
Contributor

@yassin-kammoun-sonarsource I think the decision about Node 16 was properly announced, so you can cleanup the code and remove all mentions of it https://community.sonarsource.com/t/node-js-16-is-no-longer-supported-in-the-analysis-environment/112167

@saberduck
Copy link
Contributor

The ticket mentions testing the CI against all active Node.js versions. This will really slow down our pull requests. Is it really something we want to do?

The tests would run in parallel, so they shouldn't slow down that much. However, it will consume more resources. I think testing against 18, 20, and 22 is reasonable.

@yassin-kammoun-sonarsource
Copy link
Contributor Author

The ticket mentions testing the CI against all active Node.js versions. This will really slow down our pull requests. Is it really something we want to do?

The tests would run in parallel, so they shouldn't slow down that much. However, it will consume more resources. I think testing against 18, 20, and 22 is reasonable.

Alright. I will tackle the CI changes in a separate pull request.

@yassin-kammoun-sonarsource yassin-kammoun-sonarsource merged commit 6fa23e9 into master Sep 20, 2024
15 of 16 checks passed
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.

2 participants