chore(lambda-nodejs): clean up sdk v2 references and warn about runtime updates related to sdk bundling #30099
Conversation
aws-cdk-automation
added
the
pr/needs-maintainer-review
| const externals = props.externalModules ?? defaultExternals; | ||
|
|
||
| // warn users if they are using a runtime that does not support sdk v2 | ||
| // and the sdk is not explicitly bundled | ||
| if (externals.length && isV2Runtime) { |
There was a problem hiding this comment.
what is externals.length supposed to mean/indicate?
There was a problem hiding this comment.
would it be helpful to do something like
const whatThisMeans = externals.length
so that a reader knows what's indicated by externals.length without understanding props.externals?
There was a problem hiding this comment.
externals are the packages we are explicitly not bundling. externals.length should check if that list has anything in it. externals will have the sdk package correlated to the runtime by default. Users can override or provide their own external packages.
The thought process behind this is that if a user is setting bundleAwsSDK = true or props.externalModules = [], then they likely are aware of how the bundling in the module works, and they are including the sdk in their handler code. Both of these will cause externals.length to be falsy, and not give the warning since they are avoiding sdk problems by bundling it in.
If they have not set anything, or set certain externals, we give them this warning.
There was a problem hiding this comment.
do you agree that from a readability standpoint, there'd be a benefit of having some of that information in the code, so that reviewers or people randomly looking at the code in the future can discern that without any context/knowledge of the surrounding code?
| const externals = props.externalModules ?? defaultExternals; | ||
|
|
||
| // warn users if they are using a runtime that does not support sdk v2 | ||
| // and the sdk is not explicitly bundled | ||
| if (externals.length && isV2Runtime) { |
There was a problem hiding this comment.
would it be helpful to do something like
const whatThisMeans = externals.length
so that a reader knows what's indicated by externals.length without understanding props.externals?
| @@ -156,6 +156,11 @@ environment. | |||
| When passing a runtime that is known to include a version of the aws sdk, it will be excluded by default. For example, when | |||
| passing `NODEJS_16_X`, `aws-sdk` is excluded. When passing `NODEJS_18_X`, all `@aws-sdk/*` packages are excluded. | |||
|
|
|||
| > [!WARNING] | |||
| > The NodeJS runtime of Node 16 will be deprecated by Lambda on June 12, 2024. Lambda runtimes Node 18 and higher include SDKv3 and not SDKv2. Updating your Lambda runtime from <=Node 16 to any newer version will require bundling the SDK with your handler code, or updating all SDK calls in your handler code to use SDKv3 (which is not a trivial update). Please account for this added complexity and update as soon as possible. | |||
There was a problem hiding this comment.
nit: did you mean to do <= node 16 instead of <=node 16?
| const externals = props.externalModules ?? defaultExternals; | ||
|
|
||
| // warn users if they are using a runtime that does not support sdk v2 | ||
| // and the sdk is not explicitly bundled | ||
| if (externals.length && isV2Runtime) { |
There was a problem hiding this comment.
do you agree that from a readability standpoint, there'd be a benefit of having some of that information in the code, so that reviewers or people randomly looking at the code in the future can discern that without any context/knowledge of the surrounding code?
|
Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
aws-cdk-automation
removed
the
pr/needs-maintainer-review
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
|
Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
|
Comments on closed issues and PRs are hard for our team to see. If you need help, please open a new issue that references this one. |
Closes #29836.
While sdk v2 was not being used directly in the construct, we had some remnants of its use. Added a warning for users of Node 16 who do not bundle the sdk on what it will take to update to newer node versions. Also updated integ tests to make actual sdk v3 calls as well as clean up false references to v2.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license