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.

Sign up for GitHub

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

Jump to bottom

Add Node.js 20 as a possible recommended version #4110

Merged
merged 4 commits into from Aug 30, 2023
Merged

Add Node.js 20 as a possible recommended version #4110

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
bfad53f
Run plugin integration tests against Node.js 20
yassin-kammoun-sonarsource Aug 25, 2023
9930de8
Add Node.js 20 as a possible recommended version
yassin-kammoun-sonarsource Aug 25, 2023
e504e83
Use NodeSource directly to download Node.js
yassin-kammoun-sonarsource Aug 28, 2023
17de28a
Merge branch 'master' of github.com:SonarSource/SonarJS into i4013
yassin-kammoun-sonarsource Aug 30, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions .cirrus.yml
View file
Open in desktop
Expand Up @@ -142,6 +142,7 @@
- NODE_VERSION: 14
- NODE_VERSION: 16
- NODE_VERSION: 18
- NODE_VERSION: 20
env:
SQ_VERSION: LATEST_RELEASE

Expand Down Expand Up @@ -202,7 +203,7 @@
diff_artifacts:
path: '**/target/actual/**/*'

promote_task:

Check warning on line 206 in .cirrus.yml

View check run for this annotation

Cirrus CI / Build Parsing Results

.cirrus.yml#L206 

task "promote" depends on task "ws_scan", but their only_if conditions are different

Check warning on line 206 in .cirrus.yml

View check run for this annotation

Cirrus CI / Build Parsing Results

.cirrus.yml#L206 

task "promote" depends on task "ws_scan", but their only_if conditions are different
depends_on:
- ws_scan
- build_win
Expand Down
2 changes: 1 addition & 1 deletion .cirrus/nodejs.jdk17.Dockerfile
View file
Open in desktop
Expand Up @@ -5,6 +5,6 @@ USER root

ARG NODE_VERSION=14

RUN apt-get update && apt-get install -y nodejs=${NODE_VERSION}.*
RUN curl -fsSL https://deb.nodesource.com/setup_${NODE_VERSION}.x | bash - && apt-get install -y nodejs=${NODE_VERSION}.*

USER sonarsource
4 changes: 2 additions & 2 deletions ...ript-plugin/src/main/java/org/sonar/plugins/javascript/eslint/NodeDeprecationWarning.java
View file
Open in desktop
Expand Up @@ -56,8 +56,8 @@ public class NodeDeprecationWarning {
*/
static final Version MIN_SUPPORTED_NODE_VERSION = Version.create(14, 17, 0);
static final int MIN_RECOMMENDED_NODE_VERSION = 16;
static final List<Integer> RECOMMENDED_NODE_VERSIONS = Arrays.asList(16, 18);
static final List<Integer> ALL_RECOMMENDED_NODE_VERSIONS = Arrays.asList(14, 16, 18);
static final List<Integer> RECOMMENDED_NODE_VERSIONS = Arrays.asList(16, 18, 20);
static final List<Integer> ALL_RECOMMENDED_NODE_VERSIONS = Arrays.asList(14, 16, 18, 20);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wonder whether we should get rid of these lists and rework the deprecation logic as well as the logs so that we only test whether the actual node version is an even number greater or equal than the current value of MIN_RECOMMENDED_NODE_VERSION. This means we would no longer tell explicitly which versions are recommended. Users would need to read the analyzer documentation to figure out which one, which will redirect them to Node.js LTS page.

Maybe it's not worth the effort since we will soon embed Node.js runtime. What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's not worth the effort with our current plans

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

private final AnalysisWarningsWrapper analysisWarnings;

public NodeDeprecationWarning(AnalysisWarningsWrapper analysisWarnings) {
Expand Down
14 changes: 10 additions & 4 deletions ...-plugin/src/test/java/org/sonar/plugins/javascript/eslint/NodeDeprecationWarningTest.java
View file
Open in desktop
Expand Up @@ -55,7 +55,7 @@ void test_14() {
deprecationWarning.logNodeDeprecation(14);
assertWarnings(
"Using Node.js version 14 to execute analysis is deprecated and will stop being supported no earlier than May 1st, 2023. " +
"Please upgrade to a newer LTS version of Node.js [16, 18]"
"Please upgrade to a newer LTS version of Node.js [16, 18, 20]"
);
}

Expand All @@ -70,19 +70,25 @@ void test_recommended() {
void test_15() {
deprecationWarning.logNodeDeprecation(15);
assertWarnings(
"Using Node.js version 15 to execute analysis is deprecated and will stop being supported no earlier than May 1st, 2023. Please upgrade to a newer LTS version of Node.js [16, 18]",
"Node.js version 15 is not recommended, you might experience issues. Please use a recommended version of Node.js [16, 18]"
"Using Node.js version 15 to execute analysis is deprecated and will stop being supported no earlier than May 1st, 2023. Please upgrade to a newer LTS version of Node.js [16, 18, 20]",
"Node.js version 15 is not recommended, you might experience issues. Please use a recommended version of Node.js [16, 18, 20]"
);
}

@Test
void test_17() {
deprecationWarning.logNodeDeprecation(17);
assertWarnings(
"Node.js version 17 is not recommended, you might experience issues. Please use a recommended version of Node.js [16, 18]"
"Node.js version 17 is not recommended, you might experience issues. Please use a recommended version of Node.js [16, 18, 20]"
);
}

@Test
void test_20() {
deprecationWarning.logNodeDeprecation(20);
assertWarnings();
}

@Test
void test_all_removal_dates_defined() {
var allRemovalDates = IntStream
Expand Down