search

Cloud-Automation / node-modbus

Modbus TCP Client/Server implementation for Node.JS
456 stars 169 forks source link

Update packages for node 14 and latest security fixes #291

Open iconnor opened 2 years ago

iconnor commented 2 years ago

I tried to run npm install with Node v14.17.3 and ran into this error https://github.com/serialport/node-serialport/pull/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.

image
stefanpoeter commented 2 years ago

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.

stefanpoeter commented 2 years ago

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.

iconnor commented 2 years ago

It looks like with the new include the older versions of node break:

image

Node ≥ 10 pass but Node ≤ 8 fail

iconnor commented 2 years ago

Once I limit it to current releases (12 onwards): https://en.wikipedia.org/wiki/Node.js#Releases

image

The build should pass on Travis: https://travis-ci.com/github/iconnor/node-modbus/builds/234171373

image
iconnor commented 2 years ago

@stefanpoeter, what are your thoughts on limiting the tests to supported Node versions only?

stefanpoeter commented 2 years ago

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?

iconnor commented 2 years ago

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.

iconnor commented 2 years ago

@stefanpoeter what do you think of limiting to only supported releases and updating the readme?

stefanpoeter commented 2 years ago

Hi @iconnor you are right, if the node.js version are not support then the jsmodbus version depending on those should not be supported either.

iconnor commented 2 years ago

Is this okay to merge?

stefanpoeter commented 2 years ago

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!?

iconnor commented 2 years ago

I just went for the closest patch just in case the jump to 5 introduced regressions of some sort.

stefanpoeter commented 2 years ago

Maybe @alexbuczynsky has some thoughts on this?

iconnor commented 2 years ago

@stefanpoeter and @alexbuczynsky - I saw dependabot added three PRs and just pushed a new commit that updates npm audit that also would resolve those issues.

stefanpoeter commented 1 month ago

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.