Skip to content
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

npm audit fix security vulnerability & remove unnecessary deps #540

Closed
wants to merge 2 commits into from

Conversation

coolaj86
Copy link

@coolaj86 coolaj86 commented Oct 30, 2019

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
Copy link

Coverage Status

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

1 similar comment
@coveralls
Copy link

Coverage Status

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

@Leonidas-from-XIV
Copy link
Owner

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
Copy link
Author

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
Copy link
Owner

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

@Leonidas-from-XIV
Copy link
Owner

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

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.

3 participants