Discussion: is `package-lock.json` needed?

[folletto]

This issue emerged first after we aligned Social Logos to the new build system from Gridicons:

• https://github.com/Automattic/social-logos/pull/58
• https://github.com/Automattic/social-logos/pull/57

And then recently the file was added in Gridicons, after another unrelated update:

• https://github.com/Automattic/gridicons/pull/285

In both situations, it was raised the question if the file is actually needed — by @gwwar:

we should probably add an engines section to specify the minimum node version in package.json if we want to commit a package-lock file. npm 5 wasn't paired with Node until the v8 LTS line, so folks who try to use this library may run into trouble if their project isn't at that version yet.

Looks like we added a package.lock here. Do we need it? This file does not get published with the npm package, but it is checked in locally. If we happened to have an unpinned library version, we wouldn't be able to see the errors locally if a bugfix version happened to break something, but it would show up in other projects that use gridicons as a dep.

I think we need a clear answer, and then commit the changes to all the repositories.

• Is that file needed in these repositories?
• Why?

[gwwar]

So basically the package.lock is awesome when we want consistent CI builds (which we don't have yet), or if we're a non-library project. This file isn't published, which means that clients like Calypso will pull down the latest of whatever package.json specifies.

Here's the worst case scenario: let's say we have a library dependency that is not pinned. Let's call this package foo at ^1.2.3. The package.lock sticks with 1.2.3 but a few days later Calypso reshrinkwraps and foo happens to publish 1.3.0 which breaks something. Calypso picks up the latest foo 1.3.0. Locally in this repo we don't notice what's wrong since we use the exact version in package.lock.

So two options that come to mind:

Yes, we do want it. No clear benefit in this case, but somewhat harmless as long as we pin exact packages.

• Make sure our package.json deps are pinned
• commit as normal

No, we don't:

• Add package.lock to .gitignore
• Delete it from the repository

[folletto]

To be clear: by "pinned" you mean that package.json should specifiy an exact version, correct?

[gwwar]

Yes, an exact version!

[folletto]

Ok!

Given:

• this is a small package, and an icon set
• we currently have only one dependencies (prop-types)
• all the other dependencies are devDependencies
• we have already pinned the only one that caused build issues (svg-to-pdfkit)

I'd consider the second choice to be the ideal.

We can reconsider at a later point.

[folletto]

Ok added PR to remove it from Gridicons: https://github.com/Automattic/gridicons/pull/287

[folletto]

And added PR to remove it from Social Logos too: https://github.com/Automattic/social-logos/pull/61

[folletto]

Both issues got merged. ✅ ✅

Thank you @gwwar for the always precise and clear feedback. :)

[gwwar]

Thanks for following up on this one! 💖