search

paixaop / node-sodium

Port of the lib sodium encryption library to Node.js
MIT License
351 stars 126 forks source link

Fix broken fs.unlink call and add proxy support #151

Closed michaelrommel closed 4 years ago

michaelrommel commented 5 years ago

Fixes #150 and adds proxy support for those of us, who work behind a corporate proxy.

michaelrommel commented 5 years ago

Hello, are you planning on integrating the patch? Thanks for a response!! Michael.

wzhouwzhou commented 4 years ago

Using console.log instead of console.error on errors is bad practice already, but double logging when the error is being thrown anyway makes it even worse. unlinkSync blocks the entire execution thread until the operation is finished and you've never try/catch'ed it which jeopardizes the rest of execution. The original error in #150 simply mandates the second argument of fs.unlink, none of what else you have added in this PR, more to follow... I am also against introducing https-proxy-agent as another dependency that isn't even used during runtime. There is a set way to install with npm behind a corporate proxy via npm config as this is not a niche issue; please see if those solutions work for you before trying to impose it upon all users of the library. Another solution is to introduce a "backdoor" agent accessor that allows users of the API to manually specify their own http(s) agents, assuming people install with --ignore-scripts, and then run the install script programmatically.

"Let him that would move the world first move himself." -Socrates

michaelrommel commented 4 years ago

Yeah, good points - thanks for taking the time to respond. I have deleted the PR. Thanks again!