Skip to content

Comments

Updated it to work with nodejs12.x#1

Open
DavidM42 wants to merge 3 commits intomooyoul:masterfrom
DavidM42:master
Open

Updated it to work with nodejs12.x#1
DavidM42 wants to merge 3 commits intomooyoul:masterfrom
DavidM42:master

Conversation

@DavidM42
Copy link

@DavidM42 DavidM42 commented Apr 2, 2021

  • new runtime in serverless.yml definition
  • removed check-engine for now, so it works at all with new versions
  • fixed npm script "pack" (install failed because production flag, behavior of production install and npm ls probably changed)
  • rebuilt package-lock.json

These were the changes for me to get it to work as a new deployment. Your mileage may vary.
I'm open to suggestion changes and feedback on the PR. Maybe someone has an idea how to fix the check-engine config first instead of disabling it e.g.

* new runtime in serverless.yml definition
* removed check-engine for now so it works at all with new versions
* fixed pack npm script (install failed because production flag)
* rebuilt package-lock.json
@DavidM42
Copy link
Author

DavidM42 commented Apr 3, 2021

@mooyoul Thank you for your feedback. I improved the PR and made the change less breaking.
Version check is back with node 12 and up. Also added a small fix in the serverless.yml definition format.

@DavidM42 DavidM42 requested a review from mooyoul April 3, 2021 23:58
Copy link
Owner

@mooyoul mooyoul left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution 👍 but there are some requests to approve -

package.json Outdated
},
"engines": {
"node": "^8.10.0",
"node": ">= v12.0",
Copy link
Owner

Choose a reason for hiding this comment

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

Please remove v character. and use full semver version. See https://github.com/npm/node-semver for details.

Suggested change
"node": ">= v12.0",
"node": ">= 12.0.0",

Copy link
Owner

Choose a reason for hiding this comment

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

and Please update check-engine dependency ;)

* 12.x as major version requirement
* also updated check-engine dependency
@DavidM42
Copy link
Author

DavidM42 commented Apr 4, 2021

Updated the check-engine dependency and used >= 12.x as node version check.
Any major version 12 release is supported by lambda on edge, so this check should suffice. Thanks for the node-semver link, I checked and this now is correct syntax according to node-semver docs

@DavidM42 DavidM42 requested a review from mooyoul April 4, 2021 12:46
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.

2 participants