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

ISSUE-1616: Upgrade to node 16 #1622

Merged
merged 27 commits into from
Jun 4, 2022

Conversation

keeler
Copy link
Contributor

@keeler keeler commented May 31, 2022

Summary

Fixes #1616

Bumps the node version to 16 (current active LTS version) in the docker image, package.json, etc.

Details

  • Changes to package-lock.json are from running npm audit fix and updating the @hapi/joi dependency to joi (as recommended in a warning from npm ci).

@tooomm
Copy link
Contributor

tooomm commented May 31, 2022

Hi @keeler, thanks for tackling some of our outstanding issues. ❤️
If you could please move the fixes for the two linked ones out into their own PR's, that helps a lot with reviewing and merging stuff more quickly!

The Docker and CI stuff are a lot of changes on their own, and we are not too experienced with the former.

@keeler
Copy link
Contributor Author

keeler commented Jun 1, 2022

If you could please move the fixes for the two linked ones out into their own PR's, that helps a lot with reviewing and merging stuff more quickly!

@tooomm Sure thing. I backed out the unrelated changes in this PR and updated so it only applies to issue #1616.

Opened PR #1623 for issue #1533, and opened PR #1624 for the docker image + CI refactor that I ripped out of this PR.

The Docker and CI stuff are a lot of changes on their own, and we are not too experienced with the former.

I'll update the description of #1624 and/or add some comments in a self-review to clarify the changes in detail, but another time. I'm up irresponsibly late at the moment. 😄

@keeler keeler changed the title Upgrade to node 16 and hide News panel if empty ISSUE-1616: Upgrade to node 16 Jun 1, 2022
@tooomm tooomm requested a review from ZeldaZach June 1, 2022 07:18
Dockerfile Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
@tooomm
Copy link
Contributor

tooomm commented Jun 1, 2022

Thanks for the PR split!

I'll update the description of #1624 and/or add some comments in a self-review to clarify the changes in detail, but another time. I'm up irresponsibly late at the moment.

That would be great. Hahah, get some sleep then. 😄

@keeler keeler force-pushed the ISSUE-1616-upgrade-off-node-12 branch from 27c16a5 to 6e4aa08 Compare June 2, 2022 03:55
.nvmrc Outdated
@@ -1 +1 @@
v12
16.15.0
Copy link
Member

Choose a reason for hiding this comment

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

LTS is 16.15.1

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Whoops, good catch. 395d99e

Copy link
Contributor

@tooomm tooomm Jun 2, 2022

Choose a reason for hiding this comment

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

Does it make sense to not specify the patch version to receive non-breaking fixes? Like 16.15.

@keeler keeler force-pushed the ISSUE-1616-upgrade-off-node-12 branch from 9ce3d8f to 6e4aa08 Compare June 2, 2022 04:17
@tooomm
Copy link
Contributor

tooomm commented Jun 2, 2022

There are still several warnings when running npm ci in the CI:

npm WARN EBADENGINE Unsupported engine {
npm WARN EBADENGINE   package: 'draft@1.1.1',
npm WARN EBADENGINE   required: { node: '^16.15.1' },
npm WARN EBADENGINE   current: { node: 'v16.15.0', npm: '8.5.5' }
npm WARN EBADENGINE }
npm WARN skipping integrity check for git dependency ssh://git@github.com/dr4fters/utils.git 
npm WARN deprecated @hapi/pinpoint@2.0.0: Moved to 'npm install @sideway/pinpoint'
npm WARN deprecated @hapi/formula@2.0.0: Moved to 'npm install @sideway/formula'
npm WARN deprecated @hapi/address@4.1.0: Moved to 'npm install @sideway/address'
npm WARN deprecated @babel/polyfill@7.12.1: 🚨 This package has been deprecated in favor of separate inclusion of a polyfill and regenerator-runtime (when needed). See the @babel/polyfill docs (https://babeljs.io/docs/en/babel-polyfill) for more information.
npm WARN deprecated @hapi/joi@17.1.1: Switch to 'npm install joi'
npm WARN deprecated core-js@2.6.12: core-js@<3.4 is no longer maintained and not recommended for usage due to the number of issues. Because of the V8 engine whims, feature detection in old core-js versions could cause a slowdown up to 100x even if nothing is polyfilled. Please, upgrade your dependencies to the actual version of core-js.

The first is probably save to ignore as the cached node version for 16 is still at 16.15.0 at the moment.

@keeler
Copy link
Contributor Author

keeler commented Jun 3, 2022

There are still several warnings when running npm ci in the CI:

FYI those warnings were not introduced by the changes in this PR and already existed on master (commit 0c02a92).

I fixed the @hapi/joi warning by moving the dependency to joi as recommended. Seems to work fine for me. 19ec2a3

However, the @babel/polyfill warning seems non-trivial to resolve and will need testing across different browsers. For that reason, and because the warning isn't related to the changes here, I'd argue the @babel/polyfill warning should be resolved in a separate PR. If you don't agree that should go in a separate PR, I'd appreciate advice on how to address this as browser polyfilling is not something I have much experience with. I can try to take a crack if no one else feels comfortable with it either.

@tooomm @ZeldaZach Let me know what you think.

@keeler keeler force-pushed the ISSUE-1616-upgrade-off-node-12 branch from 054e98d to 5083990 Compare June 3, 2022 04:27
keeler added a commit to keeler/dr4ft that referenced this pull request Jun 3, 2022
.github/workflows/tests.yml Outdated Show resolved Hide resolved
keeler and others added 2 commits June 3, 2022 14:09
Co-authored-by: tooomm <tooomm@users.noreply.github.com>
@tooomm
Copy link
Contributor

tooomm commented Jun 4, 2022

I fixed the @hapi/joi warning by moving the dependency to joi as recommended. Seems to work fine for me. 19ec2a3

👍

However, the @babel/polyfill warning seems non-trivial to resolve and will need testing across different browsers. For that reason, and because the warning isn't related to the changes here, I'd argue the @babel/polyfill warning should be resolved in a separate PR.

👍

@tooomm tooomm requested a review from ZeldaZach June 4, 2022 01:11
Copy link
Member

@ZeldaZach ZeldaZach left a comment

Choose a reason for hiding this comment

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

Seems reasonable, glad we're moving forward! Thanks for the change

@tooomm
Copy link
Contributor

tooomm commented Jun 4, 2022

You have to verify, merge and deploy this @ZeldaZach 😉

Same for the small change in #1623

@ZeldaZach ZeldaZach merged commit 246785e into dr4fters:master Jun 4, 2022
@keeler keeler deleted the ISSUE-1616-upgrade-off-node-12 branch June 4, 2022 20:11
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.

Bump Node version
3 participants