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

adding package-lock.json #330

Closed
hugomrdias opened this issue Feb 12, 2019 · 11 comments · Fixed by #737
Closed

adding package-lock.json #330

hugomrdias opened this issue Feb 12, 2019 · 11 comments · Fixed by #737
Assignees
Labels
exp/novice Someone with a little familiarity can pick up kind/enhancement A net-new feature or improvement to an existing feature P1 High: Likely tackled by core team if no one steps up status/in-progress In progress

Comments

@hugomrdias
Copy link
Member

Now that we are moving to travis we should talk about adding package-lock.json to the repos so the pipelines run faster.

Discuss please @ipfs/javascript-team

@hugomrdias hugomrdias added kind/enhancement A net-new feature or improvement to an existing feature status/in-progress In progress exp/novice Someone with a little familiarity can pick up P1 High: Likely tackled by core team if no one steps up labels Feb 12, 2019
@hugomrdias hugomrdias self-assigned this Feb 12, 2019
@vmx
Copy link
Member

vmx commented Feb 12, 2019

I'd prefer doing it the Rust style. Only having package locks for applications, not libraries. In our case applications would be e.g. AEgir and js-ipfs.

I could imagine that using package locks on our libraries would introduce a lot of overhead when updating dependencies as we would loose implicit updates, which are used a lot as we release so often.

@hugomrdias
Copy link
Member Author

but those implicit updates also break things often!

@jacobheun
Copy link
Contributor

but those implicit updates also break things often!

This is a rather annoying issue we hit fairly often, and it's not always easy to know where the issue is coming from.

Only having package locks for applications, not libraries. In our case applications would be e.g. AEgir and js-ipfs.

Is there a reason for this? Is it purely overhead?

I think adding package-lock.json in conjunction with Greenkeeper would be very helpful.

  • We'd have insight into any implicit changes that are coming in and causing failures.
  • Package lock updates would be handled by Greenkeeper, so we don't have to worry about overhead.
  • Library consumers still get implicit updates since package-lock.json does not get published.
  • We could switch to npm ci which would gain a lot of efficiency.

If we don't enable Greenkeeper we'd need to remember to explicitly update in range dependencies. I usually do this fairly regularly on packages I'm actively working on, but having something automatically handling that would be nice.

@vmx
Copy link
Member

vmx commented Feb 12, 2019

but those implicit updates also break things often!

Especially since implicit upgrades often break things, I think it's good to surface those as early as possible. Else you end up with lots of things to fix if you upgrade a lot of packages at the same time...

I think adding package-lock.json in conjunction with Greenkeeper would be very helpful.

...though using Greenkeeper would certainly fix this issue I'm seeing (I have it enabled on a few Greenkeeper repos already).

@hugomrdias
Copy link
Member Author

I would love to enable renovate service (greenkeeper like but with filters) where we would select only our packages and get PRs only for those, everything third party, maintainers should run npm update and update the lock.
This would be a nice balance between having a million PRs for dep updates and having control over our packages version across orgs.

@vmx
Copy link
Member

vmx commented Feb 12, 2019

I'd like to use Greenkeeper (also for personal reasons, but that's a different story), but also (or perhaps even especially) for 3rd party dependencies. It's also important to know if they break. So far in IPLD I haven't had issues with too many PRs from Greenkeeper (Greenkeeper changed how it works quite a lot in recent years). Though the IPLD modules don't have that many dependencies.

@alanshaw
Copy link
Member

Ok so as far as I can tell, package-lock.json advantages:

  1. Faster npm install
  2. Locked dependencies for developers and CI which means fewer "surprises" for developers when modules get released and break things!

and disadvantages:

  1. Library consumers get in range updates - breaking changes are pushed onto the consumer to experience and report. We lose all visibility of breaking changes in modules our projects depend on as they happen.
  2. We'll incur an overhead of merge conflicts etc. with the lock file

Greenkeeper will give us back this visibility for direct dependencies (plz correct me if I'm wrong), but we'll not have visibility on breaking changes to dependencies of dependencies.

So, if the only problem we're trying to solve here is speeding up pipelines then IMHO package-lock isn't the answer.

I'm 👍 on greenkeeper or something like it. Now we have a stable CI, keeping dependencies up to date is going to be so much easier. I'm happy to enable it for all dependencies and dial it down if it becomes unmanageable.

I'd be interested in talking through if shinkwrap is an option instead. I feel like we might be able to speed up CI as well as mitigate the pain of breaking dependencies for consumers by using this...I haven't got time just now to properly consider it but if someone has time to enumerate the advantages/disadvantages that would be helpful!

@hugomrdias hugomrdias pinned this issue Feb 20, 2019
@jacobheun
Copy link
Contributor

jacobheun commented Feb 20, 2019

Listing out advantages, disadvantages, and mitigations for the approaches here. Feel free to add/edit if something is missing.

Package Lock

Advantages

  • Faster npm install
  • Locked dependencies for developers and CI which means fewer "surprises" for developers when modules get released and break things!

Disadvantages

  • Library consumers get in range updates - breaking changes are pushed onto the consumer to experience and report. We lose all visibility of breaking changes in modules our projects depend on as they happen.
  • We'll incur an overhead of merge conflicts etc. with the lock file

Shrinkwrap

Advantages

  • Faster npm install
  • Locked dependencies for developers and CI which means fewer "surprises" for developers when modules get released and break things!
  • Library consumers get the exact package versions we released with.
  • Less prone to merge conflicts as the shrinkwrap needs to be done intentionally

Disadvantages

  • Release overhead: we'd lose in range ability to apply security updates. For example, if libp2p-circuit needs a security patch, switch and libp2p would need to be released for ipfs to get it.
  • In range updates are not supported by Greenkeeper, only out of range updates.
  • Shrinkwrap needs to be explicity done, so we need to ensure package version changes also include shrinkwrap changes

Mitigation

What can we do to mitigate the disadvantages?

Both

Devs missing in range (shrinkwrap) or dependency of dependency (package lock) updates.

Run daily Travis Cron builds that ignore lock/shrinkwrap on key projects. We shouldn't need to do this for everything, but things like js-ipfs and js-libp2p should leverage daily builds. This would also give us insight into nested dependency updates that break things for downstream users.

Package Lock

...

Shrinkwrap

...

@hugomrdias
Copy link
Member Author

+1 for the Travis Cron workaround

@alanshaw
Copy link
Member

+1 for the Travis Cron workaround

I'm concerned no-one will notice when it fails.

Release overhead: we'd lose in range ability to apply security updates. For example, if libp2p-circuit needs a security patch, switch and libp2p would need to be released for ipfs to get it.

We already do not have in range updates in our browser builds. Adding a shrinkwrap would ensure we don't forget to update them also when security fixes happen.

If js-ipfs and js-libp2p (and js-ipld?) have a shrinkwrap do we think the other projects can have package-locks?

@achingbrain
Copy link
Member

We been talking about this again. The current plan is to have a package-lock.json in the master of a project (no merge, only update) which gets converted into npm-shrinkwrap.json and/or yarn.lock files in releases.

That way it's trivial to re-create the shrinkwrap file from release tags and people installing ipfs get the same dependency tree that passed CI during the release process.

@hugomrdias hugomrdias mentioned this issue Feb 23, 2021
3 tasks
@hugomrdias hugomrdias unpinned this issue Feb 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
exp/novice Someone with a little familiarity can pick up kind/enhancement A net-new feature or improvement to an existing feature P1 High: Likely tackled by core team if no one steps up status/in-progress In progress
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants