Skip to content

Conversation

@julien-nc
Copy link
Member

Those files are local and don't need to be tracked by the repo.

@tacruc
Copy link
Collaborator

tacruc commented Feb 22, 2020

#292 (comment)

@julien-nc
Copy link
Member Author

Ok thanks for pointing to the comment.

Still it's a bit annoying since it seems only required to avoid the automatic npm "test" to fail. I would prefer to get rid of those and to make changes in the proper place to generate them before calling npm ci. What do you think?

@tacruc
Copy link
Collaborator

tacruc commented Feb 22, 2020

I have no preference, but maybe @gary-kim or @ChristophWurst pointing this out at the first time.

@gary-kim
Copy link
Member

gary-kim commented Feb 22, 2020

The point of dependency lock files are to ensure everyone has the exact same set of dependencies installed, no matter their environment. They are meant to be in the repo. They help make sure there are never any bugs that arise from having a different version of a dependency, speed up installing dependencies, and also help with security by storing the hash of all dependencies.

The documentation for the files specify that they are meant to be committed into source control.
package-lock.json documentation && composer.lock documentation

There's also some documentation about the reasons for lockfiles in npm here

Whenever updates are required for dependencies for Javascript, npm, or in our case, dependabot, will automatically update the package-lock.json file as well. The same will happen for composer dependencies as well.

EDIT: Just realized, that sounds a little more confrontational than I was intending. Just trying to be constructive, sorry if I come off a bit harsh.

@julien-nc
Copy link
Member Author

@gary-kim Ok thanks for the explanation. So we should not pay attention to the fact we will commit them often? We just commit them and that's it?

@gary-kim
Copy link
Member

Normal changes should not be updating the lockfiles unless they involve changing dependencies. The fact that the app store template includes composer update as part of the setup process is strange. @ChristophWurst do you know why that's there?

maps/Makefile

Lines 79 to 83 in c5e1f99

php $(build_tools_directory)/composer.phar install --prefer-dist
php $(build_tools_directory)/composer.phar update --prefer-dist
else
composer install --prefer-dist
composer update --prefer-dist

@julien-nc
Copy link
Member Author

Thanks a lot for looking into it!

It might just be something to fix in the Makefile indeed.

@ChristophWurst
Copy link
Member

@ChristophWurst do you know why that's there?

Good catch. That doesn't seem right. Please fix the Makefile and also submit that for the template.

Copy link
Member

@ChristophWurst ChristophWurst left a comment

Choose a reason for hiding this comment

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

No, they should be commited. The will only change when you add/update/delete a dependency.

Signed-off-by: Julien Veyssier <eneiluj@posteo.net>
@julien-nc julien-nc changed the title remove local composer and npm metadata files and put them in .gitignore update composer and npm metadata files Feb 24, 2020
@julien-nc
Copy link
Member Author

julien-nc commented Feb 24, 2020

Thanks for #314.

So I reverted everything on this branch, merged with master and ran make which updated package-lock.json. All clear?

@gary-kim
Copy link
Member

gary-kim commented Feb 25, 2020

That's strange. What version of npm and node do you have installed? My system doesn't make that change.

@julien-nc
Copy link
Member Author

Node version is v10.15.2 and npm is 5.8.0.

After upgrading npm to 6.13.7, package-lock.json is not changed anymore by running make.

Should we close this or is there anything left to do?

@tacruc tacruc closed this Feb 25, 2020
@ChristophWurst ChristophWurst deleted the remove-lock-files branch February 26, 2020 07:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants