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.

Sign up for GitHub

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

Jump to bottom

Upgrade to Node v22 #1047

Open
wants to merge 12 commits into
base: main
Choose a base branch
from
Open

Upgrade to Node v22 #1047

wants to merge 12 commits into from

Conversation

meissadia
Copy link
Collaborator

@meissadia meissadia commented Nov 6, 2024 •
edited
Loading

Close #1040

Changes

  • [e2e] Update tests for View Institution Profile that were missed as part of [View Institution profile] Update language #1013
  • [fix] [View Institution Profile] Display alert when sbl_institution_types is null or []
  • Updated .nvmrc to use v22
  • Updated package.json -> engines to use Node v22
  • Update Github actions to use Node v22
  • Updated Dockerfile to use new node-js-alpine:3.20 base image

How to test this PR

  1. Pull branch
  2. nvm use
  3. yarn start to easily launch all backend services
  4. yarn docker-build-and-run to run the container version of the Frontend
  5. Update your .env file to run tests against the container version 
    SBL_PLAYWRIGHT_TEST_TARGET="http://localhost:8085"
  6. yarn playwright test --workers 4
  7. Verify all tests pass

billhimmelsbach
billhimmelsbach approved these changes Nov 8, 2024
View reviewed changes
Copy link
Contributor

@billhimmelsbach billhimmelsbach left a comment

Choose a reason for hiding this comment

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

Awesome!

billhimmelsbach
billhimmelsbach requested changes Nov 8, 2024
View reviewed changes
Copy link
Contributor

@billhimmelsbach billhimmelsbach left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Actually, could you update the package.json required version as well?

EDIT: Hmm, we'll also want to update the Dockerfile as well.

EDIT 2: We'll also want to update the node version in the image. I'll give ya a call about that one.

@meissadia
Copy link
Collaborator Author

meissadia commented Nov 8, 2024 •
edited
Loading

Hmm, we'll also want to update the Dockerfile as well.

We'll also want to update the node version in the image.

@billhimmelsbach

  • Update package.json
  • Update Dockerfile
    • Changing this to anything other than node18 (node20, node21, node22) fails.
    • I've searched for other examples of this type of transformation, searched for a list of valid values for this -t <VERSION> argument, but I'm not finding anything that helps clarify my confusion.
    • Screenshot 2024-11-08 at 12 59 12 PM
  • Update image
    • I'm guessing this would be in the source image this container is based on? So will try to find that once I regain access.

@meissadia meissadia mentioned this pull request Nov 12, 2024
Merged
3 tasks
@meissadia
Copy link
Collaborator Author

meissadia commented Nov 13, 2024 •
edited
Loading

@billhimmelsbach I still don't know what to do about the build of import-meta-env but things seem to be working as-is.

It looks like the latest version of Node supported by pkg is v18 and the source repo is archived as of the start of 2024 so no further updates.
https://www.npmjs.com/package/pkg-fetch#binary-compatibility

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tanner-ricks tanner-ricks Awaiting requested review from tanner-ricks tanner-ricks is a code owner

@billhimmelsbach billhimmelsbach Awaiting requested review from billhimmelsbach billhimmelsbach is a code owner

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Upgrade to Node 22
2 participants
@meissadia @billhimmelsbach