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

Node18 patch arique #2316

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

Node18 patch arique #2316

wants to merge 16 commits into from

Conversation

Arique1104
Copy link
Collaborator

@Arique1104 Arique1104 commented Dec 1, 2023

Fixes StateVoicesNational#3

Description

Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context. List any dependencies that are required for this change, and any blockers that make your change a WIP

Checklist:

  • I have manually tested my changes on desktop and mobile
  • The test suite passes locally with my changes
  • If my change is a UI change, I have attached a screenshot to the description section of this pull request
  • My change is 300 lines of code or less, or has a documented reason in the description why it’s longer
  • I have made any necessary changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works
  • My PR is labeled [WIP] if it is in progress

lambda.js Outdated
@@ -1,5 +1,4 @@
"use strict";
const AWS = require("aws-sdk");
"use strict";;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Interesting. I should remove that extra semi-colon that came on after the migration commands.

@@ -4,8 +4,8 @@
"description": "Spoke",
"main": "src/server",
"engines": {
"node": ">=16.18.0",
"npm": "8.19.2"
"node": ">=20.0.0",
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This was a critical lesson learned! Some of our features do have have support in version 21, and so choosing version20 is our main priority, but no higher!

@@ -3,7 +3,8 @@ import { unzipPayload } from "../../../workers/jobs";
import { getConfig, hasConfig } from "../../../server/api/lib/config";
import { gunzip } from "../../../lib";

import AWS from "aws-sdk";
import { getSignedUrl } from "@aws-sdk/s3-request-presigner";
import { PutObjectCommand, S3 } from "@aws-sdk/client-s3";
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Why wasn't something similar added to mobile-commons document up top?

@@ -1,5 +1,4 @@
import request from "request";
import aws from "aws-sdk";
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I've reviewed this document and see no aws method being used or replaced.

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.

Nodejs 20 Upgrade
1 participant