search

Leonidas-from-XIV / node-xml2js

XML to JavaScript object converter.
MIT License
4.89k stars 605 forks source link

npm audit fix security vulnerability & remove unnecessary deps #540

Closed coolaj86 closed 4 years ago

coolaj86 commented 5 years ago

I saw that this is using util.promisify, which brings in a mountain of dependencies, and thought perhaps it would be a good strategy to use Node's native util.promisify instead, requiring only those who need older platform support to deal with the security burden all of those dependencies.

I also ran npm audit fix because npm complained about high severity security risks in the dependencies.

coveralls commented 5 years ago

Coverage Status

Coverage remained the same at 97.701% when pulling 3a8ffa1189cb8988d0b62eb940ff2774a1978044 on solderjs:remove-cruft into aefc64af9e9badbd0aade00814548f2ea33c9b4e on Leonidas-from-XIV:master.

coveralls commented 5 years ago

Coverage Status

Coverage remained the same at 97.701% when pulling 3a8ffa1189cb8988d0b62eb940ff2774a1978044 on solderjs:remove-cruft into aefc64af9e9badbd0aade00814548f2ea33c9b4e on Leonidas-from-XIV:master.

Leonidas-from-XIV commented 5 years ago

I used Node's native promisify for one release at which point people complained that dropping backwards compatibility is an incompatible change and should require a major release. Haven't gotten around to cut a new release.

coolaj86 commented 5 years ago

I hear that... but for my own stuff I just put a nice message that says "looks like you've got an old version of node, run this npm install --save whatever-polyfill"

I semver according to API-compatibility with a graceful degradation mindset that someone, who as obviously already run some command to get updated packages outside of package-lock.json, isn't realistically very inconvenienced by having to re-add the package that that tool has just removed.

Granted, npm is much more aggressive than it used to be in those regards and I often end up with this strange mix of npm deleting things from node_modules, but then not actually updating the things I expected to update...

Anyway, I'm not suggesting you should do the same, just speaking power to dropping deps. :)

Leonidas-from-XIV commented 4 years ago

The next version will drop the dependency again, it was just a temporary measure to not break existing code.

Leonidas-from-XIV commented 4 years ago

Now that #546 is merged, we got the best of both worlds, I believe.