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

build(nodejs): bump minimal nodejs and npm versions #2879

Merged
merged 1 commit into from
Dec 11, 2023

Conversation

outSH
Copy link
Contributor

@outSH outSH commented Nov 13, 2023

  • Add script tools/bump-package-engines.ts to update minimal node and npm
    versions in all cacti packages.
  • Set minimal node to 18 and npm to 8 in all cacti packages.
  • Add env variable NODEJS_VERSION in CI scripts to centralize nodejs setup.
  • Change default nodejs in CI to v18.18.2
  • Minor formatting fixes - sorted package.json, removed whitespaces.
  • Use socket.io-client-fixed-types in sawtooth connector to fix ESM import error
  • Change node-fetch to 2.7.0 (still supported) in ubiqity connector to fix
    ESM import error
  • Use explicit 127.0.0.1 instead of localhost in many source files. NodeJS 18
    prefers ipv6 over ipv4 and that caused some troubles when localhost
    was used.
  • Run codegen to update file structure.
  • Replace ts-ignore with ts-expect-error and add description to fix
    es-lint errors. Fix formatting issues found by the linter.

Signed-off-by: Michal Bajer [email protected]

Pull Request Requirements

  • Rebased onto upstream/main branch and squashed into single commit to help maintainers review it more efficient and to avoid spaghetti git commit graphs that obfuscate which commit did exactly what change, when and, why.
  • Have git sign off at the end of commit message to avoid being marked red. You can add -s flag when using git commit command. You may refer to this link for more information.
  • Follow the Commit Linting specification. You may refer to this link for more information.

Character Limit

  • Pull Request Title and Commit Subject must not exceed 72 characters (including spaces and special characters).
  • Commit Message per line must not exceed 80 characters (including spaces and special characters).

A Must Read for Beginners
For rebasing and squashing, here's a must read guide for beginners.

@outSH outSH force-pushed the bump_nodejs_version_Pr branch 9 times, most recently from 75dfa1c to 0c6ef24 Compare November 17, 2023 14:06
@outSH
Copy link
Contributor Author

outSH commented Nov 17, 2023

I think there are no more issues in CI caused by node update so it's ready for a review.

@outSH outSH marked this pull request as ready for review November 17, 2023 15:08
Copy link
Contributor

@petermetz petermetz left a comment

Choose a reason for hiding this comment

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

@outSH This is GREAT! Thank you so much!

The one nit I have strictly for future PRs, (so not this one): If you have a bunch of white-space changes in the diff, please try to split those out into a separate PR that does nothing but those white-space changes. This way the review is easier for huge PRs like this one (236 files in the diff!!!)

I know it's hard because a lot of times automated formatting gets applied without me even realizing that it had happened and then I end up with a huge diff that takes very long to untangle line-by-line after the fact because now it's mixed together with my actual changes so I can't just bulk revert everything.

So yeah, not the end of the world either way (and definitely don't waste time re-doing this PR) but I just wanted to put down a little reminder here for all of us in this regard.

LGTM!

P.S.: Sorry for the slow reviews as always!

image

Copy link
Contributor

@izuru0 izuru0 left a comment

Choose a reason for hiding this comment

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

LGTM

- Add script `tools/bump-package-engines.ts` to update minimal node and npm
    versions in all cacti packages.
- Set minimal node to 18 and npm to 8 in all cacti packages.
- Add env variable `NODEJS_VERSION` in CI scripts to centralize nodejs setup.
- Change default nodejs in CI to v18.18.2
- Minor formatting fixes - sorted package.json, removed whitespaces.
- Use socket.io-client-fixed-types in sawtooth connector to fix
    ESM import error
- Change node-fetch to 2.7.0 (still supported) in ubiqity connector to fix
    ESM import error
- Use explicit 127.0.0.1 instead of localhost in many source files. NodeJS 18
    prefers ipv6 over ipv4 and that caused some troubles when localhost
    was used.
- Run codegen to update file structure.
- Replace ts-ignore with ts-expect-error and add description to fix
    es-lint errors. Fix formatting issues found by the linter.

Signed-off-by: Michal Bajer <[email protected]>
@outSH outSH force-pushed the bump_nodejs_version_Pr branch from 0c6ef24 to 69a996b Compare December 11, 2023 16:32
@petermetz petermetz merged commit 3371772 into hyperledger-cacti:main Dec 11, 2023
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.

3 participants