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

Unpin every dependency #3186

Closed
alcuadrado opened this issue Nov 7, 2019 · 13 comments
Closed

Unpin every dependency #3186

alcuadrado opened this issue Nov 7, 2019 · 13 comments
Labels
1.x 1.0 related issues Discussion Stale Has not received enough activity

Comments

@alcuadrado
Copy link

This project has lots of pinned dependencies (i. e. exact version defined in the dependencies entries). This is not good in a library and should be removed.

Here's why I'm convinced we should do that.

Why is it bad?

  1. It leads to dependencies duplication, which can heavily impact bundle sizes when using web3 on the web.
  2. It prevents users to get new fixes.
  3. It makes it super complicated for us to push fixes to multiple versions of web3. See Cannot install on Node v13 #3181 for an example.

What about all the people pinning their dependencies?

Pinning dependencies is recommended practices in many cases, and for a reason. It makes your builds (more) reproducible, and prevents malicious actors from pushing exploits into your application.

The keyword here is "application". When building an app, and not a library, you are responsible for bundling it and need more control over your dependencies. You can do whatever you want, as nobody will use it as a dependency. Of course, this makes you are responsible for building it, deploying fixes when appropriate, and making it secure. This things are hard, but that's how the npm ecosystem is designed.

Pinning dependencies in a library is overstepping, removing the user from that needed flexibility, and leading to the problems I mentioned.

@nivida
Copy link
Contributor

nivida commented Nov 7, 2019

@alcuadrado I totally agree with you. Only Ethers has to be kept pinned until we have updated our AbiCoder test cases etc. This because of breaking changes in the AbiCoder of ethers (introduced with version 4.0.0-beta.4).

@alcuadrado
Copy link
Author

alcuadrado commented Nov 7, 2019

A side effect of unpinning dependencies in a library is that using package-lock.jsons in the CI gives you a different experience than your users'. npm and yarn only care about lock files when you install the project with an npm/yarn install, not when you install it as a dependency.

We don't use package-lock.jsons in EthereumJS nor in Buidler.

@cgewecke
Copy link
Collaborator

cgewecke commented Nov 7, 2019

A side effect of unpinning dependencies in a library is that using package-lock.jsons in the CI gives you a different experience than your users'. npm and yarn only care about lock files when you install the project with an npm/yarn install, not when you install it as a dependency.

@alcuadrado Out of curiosity, what is your view of this issue at Truffle? There, Micah Zoltu is pressing to pin deps in library-like packages for reproducibility reasons.

Is a fair summary that by unpinning, Web3 would say: 'this lib assumes its primary users will bundle it and deploy as a dapp' vs. 'this lib assumes its primary users will include it as a dep in an npm module'?

@alcuadrado
Copy link
Author

@alcuadrado Out of curiosity, what is your view of this issue at Truffle? There, Micah Zoltu is pressing to pin deps in library-like packages for reproducibility reasons.

David gave a good explanation here trufflesuite/truffle#2555 (comment). I forgot to mention that pinning dependencies doesn't even make your installations deterministic because of transitive dependencies.

To make it deterministic you have to use shrink-wrap (npm only) or bundle everything. I consider both to be overstepping for libraries, because of the same reasons I mentioned.

Is a fair summary that by unpinning, Web3 would say: 'this lib assumes its primary users will bundle it and deploy as a dapp' vs. 'this lib assumes its primary users will include it as a dep in an npm module'?

Not really. I think the same reasoning holds for web3 as a transitive dependency. It may even be stronger there, as code duplication because of pinning in a transitive dependency is even harder to spot and deal with.

@cgewecke
Copy link
Collaborator

cgewecke commented Nov 8, 2019

@alcuadrado Ah ok I see.

I'm not opposed to unpinning although as a practical matter there's a risk that will happen is:

  • everything gets unpinned
  • a steady trickle of complaints about strange behavior caused by deps begins
  • some deps will need to be pinned.

Both approaches have drawbacks. With the transitives, it seems like you're slightly buffered by the judgement of the direct package's maintainer as well.

If you both agree unpinning is good, could this effort be combined with a strategy for regular hotfix publication (per week?). Unpinning exposes the lib (and it's users) to more things outside of its control and perhaps we should be in a position to address those things quickly.

@alcuadrado
Copy link
Author

alcuadrado commented Nov 8, 2019

  • a steady trickle of complaints about strange behavior caused by deps begins

I also thought about this. But it could only happen for direct dependencies, not transitive ones. So it shouldn't be as big of a change as it seems to be.

  • some deps will need to be pinned.

I'm not opposed to pinning when necessary. For example, I had a library break Buidler multiple times, so I did two things: 1) I pinned it and it's only dependency (both from the same author), 2) Set a cron in travis to run once per day (it can't be run more frequently, at least not for free), so if a new version of a dependency breaks a test, I get a notification pretty soon. I would have removed it because of that, but it was hard to replace.

With the transitives, it seems like you're slightly buffered by the judgement of the direct package's maintainer as well.

I don't follow.

If you both agree unpinning is good, could this effort be combined with a strategy for regular hotfix publication (per week?)

I understand the intention, but I'm not sure if this will be needed. Unpinning after having things pinned for so long may require some re-betting of the dependencies being used. But unless they are publishing new versions all the time, this should be fine.

PS: And a dependency that ships too many versions should probably be avoided. It introduces instability in the system if you don't pin it. If you pin it, you lag behind its latest version very quickly.

@cgewecke
Copy link
Collaborator

cgewecke commented Nov 8, 2019

Set a cron in travis to run once per day (it can't be run more frequently, at least not for free),

I agree that's a great idea.

With the transitives, it seems like you're slightly buffered by the judgement of the direct
I don't follow...

Maybe I'm misunderstanding something but isn't the transitive dep non-determinism moderated by the tendency of other people to pin their dependencies when things are unreliable?

I understand the intention, but I'm not sure if this will be needed

Ok I guess we'll see. Am just a little wary of this change because I've now seen a full cycle of views about how to do this over the last 2-3 years and dep management seems inherently imperfect. Being prepared for the imperfect part of unpinning (if it is) might add a small margin of safety.

@cgewecke
Copy link
Collaborator

cgewecke commented Nov 9, 2019

Have listed the pinned deps below - there aren't that many. underscore@1.9.1 occurs repeatedly and it's probably fine to float so it's left out.

Maybe following on @alcuadrado's idea of vetting we could "Unpin every trusted dependency". If they float on widely used projects or ethereum-js can influence their publication, float away. Otherwise investigate more closely.

web3-bzz

"got": "9.6.0",
"swarm-js": "0.1.39",

web3-core-promievent

"any-promise": "1.3.0",
"eventemitter3": "3.1.2"

web3-eth-subscriptions

"eventemitter3": "3.1.2",

web3-eth-abi

"ethers": "4.0.0-beta.3",

web3-eth-accounts

"any-promise": "1.3.0",
"crypto-browserify": "3.12.0",
"eth-lib": "0.2.7",
"uuid": "3.3.2",

web3-eth-ens

"eth-ens-namehash": "2.0.8",

web3-eth-iban

"bn.js": "4.11.8",

web3-providers-http

"xhr2-cookies": "1.1.0"

web3-providers-ipc

"oboe": "2.1.4",

web3-utils

"bn.js": "4.11.8",
"eth-lib": "0.2.7",
"ethjs-unit": "0.1.6",
"number-to-bn": "1.7.0",
"utf8": "3.0.0"

@alcuadrado
Copy link
Author

Maybe I'm misunderstanding something but isn't the transitive dep non-determinism moderated by the tendency of other people to pin their dependencies when things are unreliable?

I see what you mean now, but I've only seen this tendency to pin dependencies in the Ethereum community to be honest.

Have listed the pinned deps below

That's excellent!

Given that there's just a few of them, they can be analyzed case by case. Some things that should be considered are: is the lib really needed, how often they release, is it maintained?

For example, I went through a couple of them:

  • got: This is a pretty serious project, started by Sindre Sorhus. I trust they won't break things in minor/patch releases. At least not often.
  • swarm-js: This is deprecated. It's unclear to me what the future of Swarm is.
  • any-promise: No version released in the last 4 years. It doesn't change much if it's pinned or not, as I guess everyone uses that version.
  • eventemitter3: They release a new version every year, or half a year.
  • ethers: @nivida mentioned that it has to be pinned for now.
  • crypto-browserify: This package is HUGE. It'd be great to replace it with https://github.com/ethereum/js-ethereum-cryptography once it's ready.
  • eth-lib: Two years without any release. Not sure if this is actively maintained. I guess ethereumjs-util already has most/all of its functionality anyway.

@cgewecke
Copy link
Collaborator

cgewecke commented Nov 11, 2019

I've only seen this tendency to pin dependencies in the Ethereum community to be honest.

Ha ha! Yes, often seems necessary.

Thanks for looking at these, scanned a couple more..

  • any-promise: I think @nivida would like to remove this altogether in New build pipeline #3160 fwiw.
  • bn.js: Floats at ether.js
  • eth-ens-namehash: Float? Maintained by Dan Finlay (metamask). (As a sidenote, test coverage is a little light on web3's ens module.)
  • number-to-bn: No activity for 3 years (SilentCicero)
  • ethjs-unit: No activity for 3 years (SilentCicero)
  • utf-8: Infrequently published and stable - last commit > two years ago
  • xhr2-cookies: Infrequently published and stable - last commit > two years ago
  • oboe: Infrequently published and stable - last commit > a year ago
  • uuid: Well maintained and heavily used (20 mil weekly downloads). Last update ~3 mo.

@github-actions
Copy link

github-actions bot commented Jul 3, 2020

This issue is stale because it has been open 30 days with no activity. Remove stale label or comment, otherwise this issue will be closed in 7 days

@github-actions github-actions bot added the Stale Has not received enough activity label Jul 3, 2020
@cgewecke
Copy link
Collaborator

cgewecke commented Jul 3, 2020

Not stale.

@cgewecke cgewecke removed the Stale Has not received enough activity label Jul 3, 2020
@github-actions
Copy link

github-actions bot commented Aug 3, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions. If you believe this was a mistake, please comment.

@github-actions github-actions bot added the Stale Has not received enough activity label Aug 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1.x 1.0 related issues Discussion Stale Has not received enough activity
Projects
None yet
Development

No branches or pull requests

3 participants