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

feat: update node and npm #908

Merged
merged 8 commits into from
Sep 9, 2022
Merged

feat: update node and npm #908

merged 8 commits into from
Sep 9, 2022

Conversation

Westlad
Copy link
Contributor

@Westlad Westlad commented Sep 6, 2022

fixes #876
This PR updates Nightfall_3 to the latest LTS node (16.17) and the latest npm (8.15).

@pawelgrzybek
Copy link
Contributor

Two little suggestions. We can add engines field to each of the package.json files. Some dev packages use these fields and can emit some helpful alerts. Also, we can put .nvmrc to the root level. Not thet it is necessary but nvm became such a popular version manager, that a lot of users can find it helpful (that should do node -v > .nvmrc).

Engines field:
https://docs.npmjs.com/cli/v8/configuring-npm/package-json#engines

Copy link
Contributor

@daveroga daveroga left a comment

Choose a reason for hiding this comment

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

LGTM

@daveroga daveroga added the One more approval needed One reviewer has approved this PR but another is needed label Sep 8, 2022
@pawelgrzybek pawelgrzybek self-requested a review September 8, 2022 14:39
@pawelgrzybek
Copy link
Contributor

@Westlad @daveroga I would also suggest changing base Docker image from FROM node:14.17 to FROM node:16.17 in:

  • optimist.standalone.Dockerfile
  • hosted-utils-api-server/Dockerfile

After this one I am happy to approve.

@Westlad
Copy link
Contributor Author

Westlad commented Sep 8, 2022

@Westlad @daveroga I would also suggest changing base Docker image from FROM node:14.17 to FROM node:16.17 in:

  • optimist.standalone.Dockerfile
  • hosted-utils-api-server/Dockerfile

After this one I am happy to approve.

Oh, good spot. I missed those.

@pawelgrzybek
Copy link
Contributor

Personally I would also remove the part with got override. It is not in the scope of node.js update. In case we need to revert one piece of functionality or the other it is easier not to mix scopes like that. Sorry for being picky :)

@Westlad
Copy link
Contributor Author

Westlad commented Sep 8, 2022

Personally I would also remove the part with got override. It is not in the scope of node.js update. In case we need to revert one piece of functionality or the other it is easier not to mix scopes like that. Sorry for being picky :)

No, it doesn't run without this. The automated dependency check will fail so it's a necessary change for this PR, I'm not going to spend time splitting it out.

Copy link
Contributor

@israelboudoux israelboudoux left a comment

Choose a reason for hiding this comment

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

Hey @Westlad, with this upgrade our Github Actions pipeline should be upgraded too. Please, could you upgrade the node-version in the files under the folder .github/workflows/?

Thank you!

@Westlad
Copy link
Contributor Author

Westlad commented Sep 9, 2022

Hey @Westlad, with this upgrade our Github Actions pipeline should be upgraded too. Please, could you upgrade the node-version in the files under the folder .github/workflows/?

Thank you!

Good spot, thanks, @israelboudoux!

@pawelgrzybek
Copy link
Contributor

Super minor, but also the readme.md needs little change.

From:

We have tested with versions 14.15.1 and 6.14.13 of `node` and `npm`, respectively.

To:

We have tested with versions 16.17.0 and 8.15.0 of `node` and `npm`, respectively.

@Westlad Westlad dismissed israelboudoux’s stale review September 9, 2022 10:54

Changes made and approval by @druiz to dismiss

@Westlad Westlad removed the One more approval needed One reviewer has approved this PR but another is needed label Sep 9, 2022
@Westlad Westlad merged commit e1c6f1d into master Sep 9, 2022
@Westlad Westlad deleted the westlad/lts branch September 9, 2022 15:02
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.

upgrade Node to latest lts
4 participants