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

Drop Node.js 14 and 16 #3747

Merged
merged 18 commits into from
Mar 27, 2024
Merged

Drop Node.js 14 and 16 #3747

merged 18 commits into from
Mar 27, 2024

Conversation

harryzcy
Copy link
Contributor

Tick the checkbox if you understand [x]:

  • I have read and understand the pull request rules.

Description

Fixes #3746

Type of change

Please delete any options that are not relevant.

  • Other

Checklist

  • My code follows the style guidelines of this project
  • I ran ESLint and other linters for modified files
  • I have performed a self-review of my own code and tested it
  • I have commented my code, particularly in hard-to-understand areas
    (including JSDoc for methods)
  • My changes generate no new warnings
  • My code needed automated testing. I have added them (this is optional task)

Screenshots (if any)

Please do not use any external image service. Instead, just paste in or drag and drop the image here, and it will be uploaded automatically.

@harryzcy
Copy link
Contributor Author

Is there anything blocking on this?

@chakflying
Copy link
Collaborator

Even though they are officially out of support, there are still many devices out there using old versions. I think I don't see the need to drop testing for them unless there is a feature that we really need only in newer versions.

@CommanderStorm CommanderStorm mentioned this pull request Jan 24, 2024
1 task
@CommanderStorm
Copy link
Collaborator

@harryzcy
Thank you for this contribution.
Currently, I don't really see a reason to force our users to upgrade.
If there are (security/other) reasons which we are missing, we would love to hear them either in this issue (if not security related) or as a security advisory if it is.

Note that we will likely update to node >=16 as part of the gamedig-5 update as mentioned in #4414

Personal note:
We might have to change this to only support the versions of node in security support, depending what comes out of (^= we will see in a few years) the EU Product Liability Directive1 and depending on what "commercial" really means.

Footnotes

  1. See
    https://fosdem.org/2024/schedule/event/fosdem-2024-3683-the-regulators-are-coming-one-year-on/ for a good introduction to this topic

@CommanderStorm CommanderStorm added this to the 2.0.0 milestone Mar 20, 2024
Saibamen

This comment was marked as resolved.

CommanderStorm and others added 3 commits March 20, 2024 14:31
Co-authored-by: Adam Stachowicz <saibamenppl@gmail.com>
Co-authored-by: Adam Stachowicz <saibamenppl@gmail.com>
@CommanderStorm

This comment was marked as resolved.

@Saibamen

This comment was marked as resolved.

CommanderStorm and others added 2 commits March 20, 2024 17:30
CommanderStorm

This comment was marked as resolved.

@CommanderStorm
Copy link
Collaborator

I think we have discussed that we need to drop node 14+16 for v2 in #4557

@chakflying @louislam any concearns about merging this PR?

@louislam
Copy link
Owner

npm@9 should be not necessary anymore.

@chakflying
Copy link
Collaborator

chakflying commented Mar 26, 2024

To summarize, moving to

≥ v16 gives us:

≥ v18 gives us:

  • croner 8.0
  • Simplifying of the testing code

@Zaid-maker
Copy link
Contributor

Louis is right npm@9 not necessary

@CommanderStorm
Copy link
Collaborator

CommanderStorm commented Mar 27, 2024

I removed npm@9

just to recap: npm@9 was added because of the lockfile version of v3, right?

@Zaid-maker
Copy link
Contributor

Zaid-maker commented Mar 27, 2024

I removed npm@9

just to recap: npm@9 was added because of the lockfile version of v3, right?

no i don't think so it was added to cap the installation of npm to 9 because of npm version 10 has breaking change and comes with node.js 20 that's why #3670

@CommanderStorm
Copy link
Collaborator

CommanderStorm commented Mar 27, 2024

What breaking changes? ci-name1 is not used and the other changes are not really relevant to us (except if we install npm@latest as before ^^)

Footnotes

  1. https://docs.npmjs.com/cli/v10/using-npm/changelog#1000-2023-08-31

@louislam
Copy link
Owner

As I remembered, Node.js 14 or 16 on GitHub Actions came with npm 6 or 7 before which was too old. So I added the @latest tag in order to update the npm to the latest version (v9 at that moment).

But later, the @latest became v10 which is not compatible with Node.js 14/16, so I locked it to @9.

@louislam louislam merged commit 88187b6 into louislam:master Mar 27, 2024
17 checks passed
@harryzcy harryzcy deleted the drop-node-14-16 branch March 27, 2024 23:49
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.

Should node 14 really be supported?
6 participants