-
Notifications
You must be signed in to change notification settings - Fork 84
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
Removal of Node 6 #783
Removal of Node 6 #783
Conversation
* Remove 6 from travis - add 8 * Update minimum node engine * Update dependencies to latest Node 8
@@ -38,31 +38,31 @@ | |||
"watch": "watch 'npm test && npm run lint' ./lib ./test" | |||
}, | |||
"dependencies": { |
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.
Are the dependencies and devDependencies changed motivated by the node 12 addition? In other words, if the modules are not upgraded this way node 12 job in travis fails?
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.
No, as summarized in the intro, they have been updated to the latest supported version for node 8 - this closes several outstanding vulnerabilities which were present due to using outdated version.
I had assumed that the older versions were being kept for node 6 support.
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.
Please add the changes to CHANGES_NEXT_RELEASE in this style:
- Upgrade from node-uuid ~1.4.1 to uuid ~3.3.2
@@ -1,9 +1,9 @@ | |||
language: node_js |
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.
A line in CHANGES_NEXT_RELEASE about the removal of Node 6 should be include. For homogeneity, I'd suggest to use the same in telefonicaid/iotagent-json#400:
Set Nodejs 8 as minimum version in packages.json (effectively removing Nodev6 from supported versions)
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.
Fixed: ab10c3b
package.json
Outdated
"mongodb": "3.1.8", | ||
"mongoose": "5.3.6", | ||
"moment": "~2.24.0", | ||
"moment-timezone": "^0.5.25", |
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.
^ -> ~, please
IOTA Node Lib version 2.9.0 has been just released, so this PR gets "unblocked" and can progress. Please have a look to new comments and solve the conflicts on CHANGES_NEXT_RELEASE. |
Merge branch 'master' into feature/node-6
CHANGES_NEXT_RELEASE
Outdated
@@ -0,0 +1,2 @@ | |||
- Upgrade from node-uuid ~1.4.1 to uuid ~3.3.2 |
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 was only an example to show style. In fact, it is not correct this time as uuid didn't change in pacakges.json in this PR. Sorry for not being clearer...
I'll try to provide a real example for a dependency and another for dev dependency:
- Upgrade async dependency from 2.6.1 to 2.6.2
- Upgrade mocha dev dependency from 5.2.0 to 6.1.4
All changes in packages.json dependencies and dev dependencies should be included this way in CHANGES_NEXT_RELEASE.
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.
Fixed 50476d3
package.json
Outdated
}, | ||
"devDependencies": { | ||
"coveralls": "~3.0.2", | ||
"coveralls": "^3.0.3", |
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.
^ -> ~, pls
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.
Fixed 6162b64
We prefer to include the ~ detail in the changelog. However, in order to simplify your work ;) I include bellow the new version of CNR with this taken into account. Just copy-paste it and upgrade the PR.
|
Fixed 6cc8714 |
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.
LGTM. Thanks!
Fixes #700