-
Notifications
You must be signed in to change notification settings - Fork 42
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
fix: remove support for Node.JS 10.x #459
Conversation
40f1859
to
bda59f7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like Node 10 references here that should also be updated
Line 110 in 8a55f32
"node": ">= 10.13.0 <13 || >=13.7.0" aws-rfdk/packages/aws-rfdk/package.json
Line 210 in 8a55f32
"node": ">= 10.13.0 <13 || >=13.7.0" aws-rfdk/tools/pkglint/lib/rules.ts
Line 632 in 8a55f32
expectJSON(this.name, pkg, 'engines.node', '>= 10.13.0 <13 || >=13.7.0'); "node": ">= 10.13.0 <13 || >=13.7.0"
bda59f7
to
f0751e6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, I only found one spot where we're referencing node 10 instead of node 12 in a message that pkglint prints out. I'm also wondering if we should be running npx yarn install
so it can update the yarn.lock
file. I see this line in there:
"@types/node" "^10.17.60"
tools/pkglint/lib/rules.ts
Outdated
@@ -376,11 +376,11 @@ export class NodeCompatibility extends ValidationRule { | |||
|
|||
public validate(pkg: PackageJson): void { | |||
const atTypesNode = pkg.getDevDependency('@types/node'); | |||
if (atTypesNode && !atTypesNode.startsWith('^10.')) { | |||
if (atTypesNode && !atTypesNode.startsWith('^12.')) { | |||
pkg.report({ | |||
ruleName: this.name, | |||
message: `packages must support node version 10 and up, but ${atTypesNode} is declared`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
message: `packages must support node version 10 and up, but ${atTypesNode} is declared`, | |
message: `packages must support node version 12 and up, but ${atTypesNode} is declared`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch. yarn.lock
is updated now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you include your changes in that most recent push? I don't see this rule or the yarn.lock updated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right. Forgot to add files to commit.
72141f5
to
ac12361
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good, I don't see anything that needs updating, but it seems you have a conflict in your yarn.lock so you'll need to rebase your changes and push them. I'll approve after that.
ac12361
to
1ae5059
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good!
I only see the one reference to "@types/node@^10.17.60"
that @horsmand mentioned in our yarn.lock
file and it looks like it's a dependency of @aws-cdk/cloudformation-diff
. We can force it to resolve to @types/node@12.x.x
by adding an entry in the resolutions in the root package.json
:
"resolutions": {
// ...
"**/@aws-cdk/cloudformation-diff/**/@types/node": "^12.8.3"
},
These are just the typings though, so not really necessary. I checked out @aws-cdk/cloudformation-diff
, changed it to depend on "@types/node": "^12.18.3"
, then built and tested it and it succeeded, so I think it's safe to assume it's not relying on things that are incompatible between Node 10 and 12.
Problem
Node.js 10 is not supported anymore since April 10, 2021
We need to update it with newer library.
Solution
Replaced nodejs10 with nodejs14 in Docker file for lambda layer.
Also was changed to take
amazonlinux
frompublic.ecr
for avoiding limit hit.Testing
Was build image and deployed on local account.
Was verified that lambda layer is using
nodejs12.x, nodejs14.x
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license