-
Notifications
You must be signed in to change notification settings - Fork 474
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
Reduce project dependencies. #361
Comments
I'd happy to replace fs-extra, but yargs would be hard. |
I'm not sure what you mean by that. The only piece which could be considered compile time is |
Hmm, I'm not sure what I was thinking with that particular comment. 🤪 |
Works towards resolving ethereum#361. Yargs brings in ~26 packages while commander brings in 1.
Works towards resolving ethereum#361. Yargs brings in ~26 packages while commander brings in 1.
Works towards resolving ethereum#361. Yargs brings in ~26 packages while commander brings in 1.
Just checked and it appears yargs is actually gone already. 🎉 There is still |
Do you want to submit a PR for replacing fs-extra? I assume we only use one or two helpers, and having it inline defined would not result in more than 20 extra lines of code? |
I'm considering it. If someone beats me to it though that would make me happy. 😉 |
If function checkPath (pth) {
if (process.platform === 'win32') {
const pathHasInvalidWinCharacters = /[<>:"|?*]/.test(pth.replace(path.parse(pth).root, ''))
if (pathHasInvalidWinCharacters) {
const error = new Error(`Path contains invalid characters: ${pth}`)
error.code = 'EINVAL'
throw error
}
}
}
export function ensureDirSync(dir) {
checkPath(dir)
return fs.mkdirSync(dir, { mode: 0o777, recursive: true })
} I don't have a dev environment setup at the moment so I can't do this myself, but it should be a really easy change:
|
DO we really need to check if the path is valid? |
For context, the above code was a direct copy (with a couple simplifications) of the dependency I'm proposing to remove. It is certainly possible that in the place it is being used, the check is unnecessary. |
This should remove ~16 of the 25 dependencies of this project, which reduces the attack surface area and gets us down to only direct dependencies I believe. See ethereum#361 (comment) for additional discussion. I went ahead and removed the path check per thought from @chriseth, but I'm happy to put it back in if desired.
This is a minor quality of life thing, but at the moment the NPM
solc
package brings with it 77 dependencies. Given the simplicity of this project (just a wrapper around solc binaries) and the fact that almost all of the work with this library occurs at compile time, it feels like there is an opportunity to reduce the dependencies thatsolc
brings into a project using it.Can any of the dependencies be moved over to
devDependencies
? Can any of the dependencies be replaced with just some inline code?Looking at solc in http://npm.anvaka.com/#/view/2d/solc, it appears that the number one offender is
yargs
followed byfs-extra
andkeccak
. I already have a PR out for removingkeccak
(in favor ofjs-sha3
), and it appears thatfs-extra
is used in one place and could easily be replaced with node's built-infs
package.yargs
I suspect will be harder to get rid of, though perhaps there is a similar library that doesn't bring in 58 transitive dependencies?The text was updated successfully, but these errors were encountered: