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(deps): bump node from 14 to 16 #2078

Merged
merged 1 commit into from
Dec 13, 2022
Merged

Conversation

halfwhole
Copy link
Collaborator

@halfwhole halfwhole commented Nov 14, 2022

Problem

We need to upgrade node from 14 to 16 - see #1873 for more info

Closes #1873

Solution

Bump node version from 14 to 16. With this upgrade, npm also needs to be upgraded from version 6 to 8 (or minimally 7, but the latest version of node 16 uses npm 8 by default).

Note that:

  • From npm 7 onwards, the lockfile version is bumped from 1 to 2, explaining the large size increase for package-lock.json
  • From npm 7 onwards, peer dependencies are installed by default, unlike in npm <=6 where they were not. To ignore this new requirement, we use the flag --legacy-peer-deps with npm install. Doing this merely reverts back to pre-npm-7 behaviour, so adding the flag is safe. Note that not using --legacy-peer-deps will cause issues with our current codebase; we may want to fix this in the future. See this Stackoverflow question for more info
  • From node 15 onwards, unhandled promise rejections will throw an error and terminate the node process if not handled properly (see node 15 release notes for more info). This may be a bit risky as bugs like e.g. fix: add return after making server response #2074 can cause our server to crash, so we should also merge fix: handle unhandled nodejs rejections #2080 to eliminate that risk

I've also changed the E2E test setup in ci.yml. I've removed DevExpress/testcafe-action, which doesn't work with node 16 because it npm installs testcafe independently without allowing us to use --legacy-peer-deps. Since we've already installed testcafe locally in the previous step, I figured it was enough to use a npm script to run E2E and found the testcafe action to be superfluous.

Future action items:

  • Upgrade everything here from node 16 -> 18, which extends our security support to Apr 2025
  • Upgrade serverless from node 14 -> 18. This upgrade requires us to reconfigure our lambdas, and so it should be done in one shot to minimize disruption esp. to bulk upload lambdas - see Chore/node 14 #1858 for more details
  • Remove --legacy-peer-deps

[Dev notes: these were the steps I took to update package-lock.json]

  • Upgrade to node 16 in package.json
  • Run nvm use 16
  • Run npm i --package-lock-only: this upgrades package-lock.json lockfile version from 1 to 2
  • Run npm i --legacy-peer-deps

[Note: deal with these warnings when upgrading to node v18]
Screenshot 2022-12-12 at 10 22 34 AM

Tests

  • Test on local dev
  • Test on staging

@halfwhole halfwhole force-pushed the build/deps/update-node-16 branch 5 times, most recently from 922f081 to 8843c8c Compare December 8, 2022 08:43
@halfwhole halfwhole force-pushed the build/deps/update-node-16 branch from 8843c8c to 41d52ea Compare December 8, 2022 08:51
@halfwhole halfwhole marked this pull request as ready for review December 8, 2022 09:13
Copy link
Contributor

@gweiying gweiying left a comment

Choose a reason for hiding this comment

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

Changes look fine to me, but wondering if we should spend more time to resolve the peer dependency issue more thoroughly rather than falling upon the --legacy-peer-deps option.

Good catch with the E2E installation module

@halfwhole
Copy link
Collaborator Author

Just looked a little into how to resolve the peer dependencies issue, and found that it likely requires multiple dependency upgrades for some important packages, like Joi and React, so it could be quite an undertaking. Does it sound alright to defer de-conflicting the peer deps till later? Or we can also try resolving the peer deps issue first before merging this in!

@gweiying
Copy link
Contributor

Just looked a little into how to resolve the peer dependencies issue, and found that it likely requires multiple dependency upgrades for some important packages, like Joi and React, so it could be quite an undertaking. Does it sound alright to defer de-conflicting the peer deps till later? Or we can also try resolving the peer deps issue first before merging this in!

Thanks for looking into it! Sure, that sounds reasonable

@halfwhole halfwhole merged commit 1422099 into develop Dec 13, 2022
@halfwhole halfwhole deleted the build/deps/update-node-16 branch December 13, 2022 03: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.

Upgrade to Node 16 LTS
2 participants