-
Notifications
You must be signed in to change notification settings - Fork 295
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
build(nodejs): bump minimal nodejs and npm versions #2879
Conversation
75dfa1c
to
0c6ef24
Compare
I think there are no more issues in CI caused by node update so it's ready for a review. |
There was a problem hiding this 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!
There was a problem hiding this 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]>
0c6ef24
to
69a996b
Compare
tools/bump-package-engines.ts
to update minimal node and npmversions in all cacti packages.
NODEJS_VERSION
in CI scripts to centralize nodejs setup.ESM import error
prefers ipv6 over ipv4 and that caused some troubles when localhost
was used.
es-lint errors. Fix formatting issues found by the linter.
Signed-off-by: Michal Bajer [email protected]
Pull Request Requirements
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.-s
flag when usinggit commit
command. You may refer to this link for more information.Character Limit
A Must Read for Beginners
For rebasing and squashing, here's a must read guide for beginners.