ethereum / solc-js

Javascript bindings for the Solidity compiler
https://soliditylang.org
MIT License
1.45k stars 474 forks source link

Reduce project dependencies. #361

Open MicahZoltu opened 5 years ago

MicahZoltu commented 5 years ago

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 that solc 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 by fs-extra and keccak. I already have a PR out for removing keccak (in favor of js-sha3), and it appears that fs-extra is used in one place and could easily be replaced with node's built-in fs 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?

axic commented 5 years ago

I'd happy to replace fs-extra, but yargs would be hard.

axic commented 5 years ago

the fact that almost all of the work with this library occurs at compile time

I'm not sure what you mean by that. The only piece which could be considered compile time is downloadCurrentVersion.js.

MicahZoltu commented 5 years ago

Hmm, I'm not sure what I was thinking with that particular comment. 🤪

MicahZoltu commented 3 years ago

Perhaps a solution to the yargs problem would be to have a separate package for the CLI from the library? All of my projects compile programmatically (not using the CLI), and I don't think I'm alone in following this pattern. Since yargs presumably is only used by the CLI portion, that would all go away if this project was split.

Just checked and it appears yargs is actually gone already. 🎉 There is still fs-extra which brings in 16 out of the 25 dependencies. I feel like if we can drop fs-extra this could probably be closed (though it would be cool to get rid of any of the remaining 9 dependencies if possible).

axic commented 3 years ago

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?

MicahZoltu commented 3 years ago

I'm considering it. If someone beats me to it though that would make me happy. 😉

MicahZoltu commented 2 years ago

If fs-extra was replaced by fs (built-in) then the only function missing would be ensureDirSync. Looking at fs-extra it is just an alias of makeDirSync internally. The following code I think is what would be required to remove the dependency on fs-extra entirely:

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:

  1. Remove fs-extra dependency from package.json
  2. Add the above code to solcjs.
  3. Change require('fs-extra'); to `require('fs');
chriseth commented 2 years ago

DO we really need to check if the path is valid?

MicahZoltu commented 2 years ago

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.