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

Reduce project dependencies. #361

Open
MicahZoltu opened this issue May 11, 2019 · 9 comments
Open

Reduce project dependencies. #361

MicahZoltu opened this issue May 11, 2019 · 9 comments

Comments

@MicahZoltu
Copy link
Contributor

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

axic commented Jul 4, 2019

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

@axic
Copy link
Member

axic commented Jul 4, 2019

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

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

MicahZoltu added a commit to MicahZoltu/solc-js that referenced this issue Oct 27, 2019
Works towards resolving ethereum#361.  Yargs brings in ~26 packages while commander brings in 1.
MicahZoltu added a commit to MicahZoltu/solc-js that referenced this issue Oct 27, 2019
Works towards resolving ethereum#361.  Yargs brings in ~26 packages while commander brings in 1.
MicahZoltu added a commit to MicahZoltu/solc-js that referenced this issue Oct 28, 2019
Works towards resolving ethereum#361.  Yargs brings in ~26 packages while commander brings in 1.
@MicahZoltu
Copy link
Contributor Author

MicahZoltu commented May 27, 2021

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

axic commented May 27, 2021

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

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

@MicahZoltu
Copy link
Contributor Author

MicahZoltu commented Dec 14, 2021

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

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

@MicahZoltu
Copy link
Contributor Author

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.

MicahZoltu added a commit to MicahZoltu/solc-js that referenced this issue Dec 19, 2021
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Status: Implement
Development

No branches or pull requests

5 participants
@axic @cameel @MicahZoltu @chriseth and others