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-371 - Deprecate Node.js 18 #4913

Merged
merged 5 commits into from
Nov 26, 2024
Merged

JS-371 - Deprecate Node.js 18 #4913

merged 5 commits into from
Nov 26, 2024

Conversation

ericmorand-sonarsource
Copy link
Contributor

@ericmorand-sonarsource ericmorand-sonarsource commented Nov 19, 2024

Copy link
Contributor

Choose a reason for hiding this comment

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

Hey @ericmorand-sonarsource, as mentioned during the standup, deprecating the support of a Node.js version requires also a bit of work on the Java side. In particular, we need to update the following:

static final Version MIN_SUPPORTED_NODE_VERSION = Version.create(18, 17, 0);
private static final int MIN_RECOMMENDED_NODE_VERSION = 18;
private static final List<String> RECOMMENDED_NODE_VERSIONS = List.of("^18.18.0", "^20.9.0", "^22.9.0");
private final AnalysisWarningsWrapper analysisWarnings;

Maybe we should document the process of deprecating a Node.js version to avoid tribal knowledge. Not sure where to write this down...

@@ -56,7 +56,7 @@
},
"homepage": "https://github.com/SonarSource/SonarJS#readme",
"engines": {
"node": "^18.17.0 || ^20.9.0 || >=21.1.0"
"node": ">=22"

Choose a reason for hiding this comment

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

Since we are only deprecating, not dropping support, maybe we should keep this as it was before.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This change establishes that the project needs Node 22 to be operated: you can't run the unit tests without at least Node 22.

@@ -3,7 +3,7 @@ FROM ${CIRRUS_AWS_ACCOUNT}.dkr.ecr.eu-central-1.amazonaws.com/base:j17-latest

USER root

ARG NODE_VERSION=18
ARG NODE_VERSION=22

Choose a reason for hiding this comment

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

Let's double-check with the rest of the squad whether we still want to test SonarJS against Node.js 18 on CI.

@ericmorand-sonarsource
Copy link
Contributor Author

ericmorand-sonarsource commented Nov 26, 2024

@yassin-kammoun-sonarsource , issues should be fixed:

  • NodeDeprecationWarning was updated
  • The formatting issues on documentation were fixed
  • I restored the Docker manifest to Node 18

Copy link
Contributor

Choose a reason for hiding this comment

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

I left two minor suggestions about spelling, otherwise looks good to me.

README.md Outdated Show resolved Hide resolved
docs/DEV.md Outdated Show resolved Hide resolved
Co-authored-by: Yassin Kammoun <52890329+yassin-kammoun-sonarsource@users.noreply.github.com>
Co-authored-by: Yassin Kammoun <52890329+yassin-kammoun-sonarsource@users.noreply.github.com>
@ericmorand-sonarsource ericmorand-sonarsource merged commit 1024c59 into master Nov 26, 2024
17 of 18 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