-
Notifications
You must be signed in to change notification settings - Fork 179
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
Update packages for node 14 and latest security fixes #291
base: v4.0-dev
Are you sure you want to change the base?
Update packages for node 14 and latest security fixes #291
Conversation
Can you add node.js versions 12 and 14 to the .travis.yml file? I'll be back on the desk on Thursday and open up a new branch. |
I've put your work into the v4.1-dev branch but currently travis-ci won't build, don't know why it is not working currently. |
Once I limit it to current releases (12 onwards): https://en.wikipedia.org/wiki/Node.js#Releases The build should pass on Travis: https://travis-ci.com/github/iconnor/node-modbus/builds/234171373 |
@stefanpoeter, what are your thoughts on limiting the tests to supported Node versions only? |
My thoughts: Since we did not define what versions are to be supported we are dependend on the currently used plattforms. Is there any way to determine statistics with what node.js versions this package is being used? Otherwise there is no way to tell if this change will bother current users. Is there maybe another way to support plattforms older than v12? |
If people are running versions lower than the supported ones, they are open to vulnerabilities. It is safer to mark the 2.0.1 release as the last supported version for those under v12 and require upgrades for newer features. I added a sentence in the readme to call that out. My concern is that Modbus is common in the power industry so encouraging maintainers of those systems to keep current and limit vulnerabilities is a good goal. |
@stefanpoeter what do you think of limiting to only supported releases and updating the readme? |
Hi @iconnor |
Is this okay to merge? |
It is ok to merge. I am just wondering if it should be version 4.0.7 or 4.1.0 or does it even need to be version 5 since these are kind of breaking changes!? |
I just went for the closest patch just in case the jump to 5 introduced regressions of some sort. |
Maybe @alexbuczynsky has some thoughts on this? |
@stefanpoeter and @alexbuczynsky - I saw dependabot added three PRs and just pushed a new commit that updates |
Hey @iconnor , sorry for the late reply. I will go with this but we should at least mark it a minor change. Can you increase the minor version number. Than this can be merged. Thanks for the commitment and again sorry for the long wait. |
I tried to run
npm install
with Node v14.17.3 and ran into this error serialport/node-serialport#1743, so I updated node-serialport the latest, and it resolved that issue.There were also 70 vulnerabilities (25 low, 4 moderate, 38 high, 3 critical). Mostly from lodash and some others. When I updated nyc and mocha it resolved these alerts.