-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
Comments
@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 |
A side effect of unpinning dependencies in a library is that using We don't use |
@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'? |
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.
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. |
@alcuadrado Ah ok I see. I'm not opposed to unpinning although as a practical matter there's a risk that will happen is:
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. |
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.
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.
I don't follow.
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. |
I agree that's a great idea.
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?
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. |
Have listed the pinned deps below - there aren't that many. 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
web3-core-promievent
web3-eth-subscriptions
web3-eth-abi
web3-eth-accounts
web3-eth-ens
web3-eth-iban
web3-providers-http
web3-providers-ipc
web3-utils
|
I see what you mean now, but I've only seen this tendency to pin dependencies in the Ethereum community to be honest.
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:
|
Ha ha! Yes, often seems necessary. Thanks for looking at these, scanned a couple more..
|
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 |
Not stale. |
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. |
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?
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.
The text was updated successfully, but these errors were encountered: