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

Remove Composer lock file #193

Closed
wants to merge 1 commit into from
Closed

Remove Composer lock file #193

wants to merge 1 commit into from

Conversation

allejo
Copy link
Member

@allejo allejo commented Jul 23, 2019

Do we actually need a lock file for a library? There's nothing for us to deploy so we don't need to lock in any of the dependencies. Our only dependencies are dev dependencies, which I don't think we need to lock.

@allejo allejo requested a review from a team July 23, 2019 19:50
Copy link
Contributor

@jaapio jaapio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there are a few reasons to keep the lock file.

  • Speed, composer install is way faster when using the lockfile. Since it only has to download the dependencies
  • consistency between builds, the lockfile prevents unexpected upgrades of our dev dependencies.

In this case the defined versions are quite strict so it shouldn't be a problem at this moment. But I think we might want to reintroduce it at some point. I'm missing some reasoning in your pr why you want to remove it. Just removing the lockfile without any arguments is not very plausible for me.

@allejo
Copy link
Member Author

allejo commented Jul 26, 2019

consistency between builds, the lockfile prevents unexpected upgrades of our dev dependencies.

I've got no argument against this, I agree that it'd let us replicate dev builds accurately if we need to track down some issue with our dev tools.

I think that not having a lock file, while slower, would let us actually find issues with our dev tools quicker. Since we'll always be using the latest version (per our composer.json requirements), we'd notice something amiss sooner rather than later when we update our dependencies. Removing the lock file would also allow us to remove the need for PRs like #194 and whichever new PR comes along that will update the phpstan dependency, which currently breaks with our 7.4snapshot testing. The lack of 7.4 compatability has already been fixed upstream (phpstan/phpstan#2275) and will be available in 0.11.13 or whichever the next release is.

My thinking is that as long as our composer.json requirements are strict enough to prevent pulling down major breaking changes, we'd get the benefit of not having to worry about explicitly updating our dev dependencies and just triggering new builds instead.

@jaapio jaapio requested a review from a team July 26, 2019 06:37
@jaapio
Copy link
Contributor

jaapio commented Jul 26, 2019

I have my doubts about this pr. But I would agree with a second opinion form another team member to have this merged.

I requested a review from the team. Let's see what others are thinking about this :-)

@localheinz
Copy link
Contributor

Personally, I would suggest to keep composer.lock

Also see http://naderman.de/slippy/slides/2018-01-26-composer-lock-demystified.pdf.

Copy link
Member

@mikey179 mikey179 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I always keep the composer.lock file in my projects, regardless of whether it's a library only or not. Reproducibility is a big asset I wouldn't throw away lightheartedly. I think it's also helpful for contributors - when tests work on their machine but not on Travis different versions of dev dependencies can be ruled out. It's great they want to contribute - don't burden them with figuring out problems due to different versions.

@jaapio
Copy link
Contributor

jaapio commented Jul 26, 2019

We could start using dependabot. That will help use to keep up to date?

@allejo
Copy link
Member Author

allejo commented Jul 27, 2019

I'm not adamant about removing the lock file, I'm totally fine with keeping it for speed and reproducibility of builds, so I'll close this PR.

+1 from me for giving dependabot a try if others are interested as well.

@allejo allejo closed this Jul 27, 2019
@allejo allejo deleted the remove-lock-file branch July 27, 2019 02:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants