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

Lf 3334 investigate node 16 eol sept 2023 #2739

Merged
merged 21 commits into from
Jun 29, 2023

Conversation

Duncan-Brain
Copy link
Collaborator

@Duncan-Brain Duncan-Brain commented Jun 21, 2023

Description

Upgrading Node 16 to Node 18.16.1 LTS and dealing with the issues that follow from that.

Overview of what was done:
All

  • package-lock.json rebuilt

Frontend

  • Material UI was upgraded using the fairly thorough Material UI Migration Guide and pre-made codemods
    • Codemods were run
    • Guide was followed for non code mods elligible fixes
    • some minor style changes
    • Frontend was toured by identifying all custom components that use MUI and visiting some view
  • React-Icons was fixed with new import syntax
    • import {VSCDialog} from react-icons/all --> import {VSCDialog} from react-icons/vsc

Backend

Other:
Why not use $BeforeValidate: Vincit/objection.js#77
Why I did not use objection-js-soft-delete (fix does not work in jest): alex-w0/objection-js-soft-delete#116
What made me try objection-js-soft-delete several times: griffinpp/objection-soft-delete#18
Testing out of use routes like Pesticide with Postman requires Body data in delete request (bad pattern) to test anyway comment out rejectInBodyGetAndDelete in server.js .

Jira link: LF-3334

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

  • Passes test case
  • UI components visually reviewed on desktop view
  • UI components visually reviewed on mobile view
  • Other (please explain):
  • Please run backend Jest Tests they should Pass (this is how I found many of the classes of type errors from ajv format and schema validation)
  • Frontend basic functionality seems to all be working scrolling around and testing
  • I would love it if someone could test cypress test to see if it picks anything up (@MwayaB are any tests working again?)
  • Look for:
    • Whitescreen crashes with material UI console statements on frontend
    • 400 bad requests on the backend likely due to a formatting error on a Model validation. Use .debug() on the model validation if error not visible

Checklist:

  • I have commented my code, particularly in hard-to-understand areas
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • The precommit and linting ran successfully
  • I have added or updated language tags for text that's part of the UI
  • I have added "MISSING" for all new language tags to languages I don't speak
  • I have added the GNU General Public License to all new files

@iperdomo
Copy link
Collaborator

This branch should be updated with the latest integration. Note the conflicts on packages/api/package-lock.json, packages/webapp/pnpm-lock.yaml

…ving jest test failures use of base properties outside base model, deprecated quantity_kg, fakeemail on accept invite, softdelete mergeContext return dates
This commit looked to address the jest errors happening as a result of the stricter types.
Includes:
- seeding field work task types during test
- working around the server.js function that converts dates to datetimes.
…us merge.

React icons seems to have stopped supporting the export of /all -- a minor breaking change.

Main repo package lock missed the previous commit so may have issues without it if checking out prev commit.
Package locks updated using the newest LTS that came out during this PR
@Duncan-Brain Duncan-Brain marked this pull request as ready for review June 24, 2023 03:24
Copy link
Collaborator

@iperdomo iperdomo left a comment

Choose a reason for hiding this comment

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

Great job! ... I haven't tested locally but I can imagine all the work going through all those little details.

One thing I noticed is that we didn't updated prod.Dockerfile

packages/api/src/controllers/loginController.js Outdated Show resolved Hide resolved
Forgot to remove debug statements and update dockerfiles

Did NOT touch storybook.Dockerfile
@Duncan-Brain
Copy link
Collaborator Author

@iperdomo Thanks totally forgot those two items.

I did not touch storybook.Dockerfile .. which seems to need updating as well.

@iperdomo
Copy link
Collaborator

@Duncan-Brain I was trying to produce a Docker image from webapp and got this error

#14 [build 7/7] RUN pnpm run build
#14 0.895 
#14 0.895 > [email protected] build /usr/src/app
#14 0.895 > tsc && NODE_OPTIONS=--max_old_space_size=3000 vite build
#14 0.895 
#14 9.574 src/components/Typography/index.tsx(197,13): error TS2322: Type 'true | ReactPortal | ReactChild | ReactFragment' is not assignable to type 'string | (string & ReactElement<any, string | JSXElementConstructor<any>>) | (string & Iterable<ReactNode>) | (string & ReactPortal) | undefined'.
#14 9.574   Type 'number' is not assignable to type 'string | (string & ReactElement<any, string | JSXElementConstructor<any>>) | (string & Iterable<ReactNode>) | (string & ReactPortal) | undefined'.
#14 9.623  ELIFECYCLE  Command failed with exit code 2.
#14 ERROR: process "/bin/sh -c pnpm run build" did not complete successfully: exit code: 1
------
 > [build 7/7] RUN pnpm run build:
0.895 
0.895 > [email protected] build /usr/src/app
0.895 > tsc && NODE_OPTIONS=--max_old_space_size=3000 vite build
0.895 
9.574 src/components/Typography/index.tsx(197,13): error TS2322: Type 'true | ReactPortal | ReactChild | ReactFragment' is not assignable to type 'string | (string & ReactElement<any, string | JSXElementConstructor<any>>) | (string & Iterable<ReactNode>) | (string & ReactPortal) | undefined'.
9.574   Type 'number' is not assignable to type 'string | (string & ReactElement<any, string | JSXElementConstructor<any>>) | (string & Iterable<ReactNode>) | (string & ReactPortal) | undefined'.
9.623  ELIFECYCLE  Command failed with exit code 2.
------
prod.Dockerfile:13
--------------------
  11 |     COPY ./shared/ /usr/src/shared/
  12 |     
  13 | >>> RUN pnpm run build
  14 |     
  15 |     FROM nginx:1.15
--------------------
ERROR: failed to solve: process "/bin/sh -c pnpm run build" did not complete successfully: exit code: 1

The interesting bit

9.574 src/components/Typography/index.tsx(197,13): error TS2322: Type 'true | ReactPortal | ReactChild | ReactFragment' is not assignable to type 'string | (string & ReactElement<any, string | JSXElementConstructor<any>>) | (string & Iterable<ReactNode>) | (string & ReactPortal) | undefined'.
9.574   Type 'number' is not assignable to type 'string | (string & ReactElement<any, string | JSXElementConstructor<any>>) | (string & Iterable<ReactNode>) | (string & ReactPortal) | undefined'.

…ge and fix type error and node version

After merging integration a new lockfile was needed becasue of the vulnerable packages. However

The vite override stopped the lazy loaded modules from being able to resolve.

Fixed a type error on Typography component -- unsure why this just presented itself.

Updated node in beta-export deploy script.
@Duncan-Brain
Copy link
Collaborator Author

@iperdomo I am not sure why that only just now presented itself to be honest. I fixed it though by resolving the type error in the Typography Tooltip component. That made me wonder if other utilities from our scripts might have issues and it seems like Storybook tests are no longer passing (not sure if it was this or pre-existing but hoping to determine tomorrow).

I also found that I forgot to update package-lock after merging integration (necessary because of the npm vulnerable packages). @SayakaOno there was a change to the overrides made in /webapp -- the vite override was causing my modules not to allow lazy loading. I deleted the overrides and re-ran pnpm audit fix and that specific override did not rejoin.

@kathyavini I added the node update to the deploy script thanks!

kathyavini
kathyavini previously approved these changes Jun 27, 2023
Copy link
Collaborator

@kathyavini kathyavini left a comment

Choose a reason for hiding this comment

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

I've been using this branch exclusively for several days and it's great -- super stable!! I see one deprecation warning on the frontend which I think can be its own ticket?

There's a good chance we'll have to do a one-time nvm install on the beta export server droplet before we can nvm use 18.16.1 (so no worries if it fails the first time). I just added updating the prod version of our export server to our release checklist -- hopefully our version of pm2 is compatible with Node 18.16.1 🤞 -- Is there anything else related to this update that should go on the release checklist?

And you really held firm on not updating the Docker Hub Image reference 😉 I'll open a PR once this one is merged, then.

Wonderful job on another massive undertaking 🙂

@@ -1,6 +1,13 @@
import { createTheme, ThemeProvider } from '@material-ui/core';
import { createTheme, StyledEngineProvider } from '@mui/material';
import { adaptV4Theme, ThemeProvider, Theme } from '@mui/material/styles';
Copy link
Collaborator

Choose a reason for hiding this comment

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

I see a deprecation warning with adaptV4Theme in my console, but I'm assuming that's an issue for another time, since the theme itself will need to be refactored?

Screenshot 2023-06-26 at 9 09 33 PM

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

definitely another time! If it ain't broke..

Copy link
Collaborator

Choose a reason for hiding this comment

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

Alright I'll make a ticket

This updates the package locks again. Current state of storybook is 33 tests failing. All else working
kathyavini
kathyavini previously approved these changes Jun 27, 2023
Copy link
Collaborator

@kathyavini kathyavini left a comment

Choose a reason for hiding this comment

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

I'm not getting close to only 33 failing Storybook tests, but that's an issue for another day!

@SayakaOno
Copy link
Collaborator

I confirmed that tests for the unit component ran successfully for me, too. I'm not able to run tests in integration branch, so I can't compare the result, but I know all tests were not passing before, and I guess it should be good!
Here is the result:
Screenshot 2023-06-28 at 2 22 43 AM

iperdomo
iperdomo previously approved these changes Jun 29, 2023
@Duncan-Brain Duncan-Brain dismissed stale reviews from kathyavini and iperdomo via f07b431 June 29, 2023 15:52
@iperdomo iperdomo requested review from iperdomo and kathyavini June 29, 2023 16:09
@iperdomo iperdomo merged commit 55866c3 into integration Jun 29, 2023
@iperdomo iperdomo deleted the LF-3334-investigate-node-16-eol-sept-2023 branch June 29, 2023 16:10
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.

4 participants