Check in package-lock.json or no?

[springmeyer]

My (limited) understanding is that we only want to version package-lock.json inside a git repo at an "end-of-line" or final dependency where we want to control versions tightly. For something like node-cpp-skel we don't want to check in package-lock.json. What do others think?

/cc @millzpaugh @mapsam @GretaCB

Asking since:

• We do not currently have package-lock.json in the .gitignore
• I noticed @millzpaugh checked it in for node-cpp-skel at https://github.com/mapbox/node-cpp-skel/commit/e829302241dc528df86681b12f0190088a235bf3#diff-32607347f8126e6534ebc7ebaec4853d

@millzpaugh was this intended?

[jfirebaugh]

If npm+package-lock.json works the same as yarn+yarn.lock (and I imagine it does), then the lockfile takes effect only when you're running npm install within the package repository, for development of the package itself. When the package is listed as a dependency in some downstream package or application, it's own lockfile is ignored. This implies two things:

• When you want control over which versions of your dependencies get installed when your package is itself used as a dependency, that must go in package.json.
• There's a convincing argument that lockfiles should always be checked in.

[mapsam]

Agree with @jfirebaugh I think we should publish lockfiles - as we move into at least npm v5 or greater, they'll become regular parts of our workflows. Plus, it seems like we don't need to worry about them being published. from the NPM docs on package-lock.json files:

One key detail about package-lock.json is that it cannot be published, and it will be ignored if found in any place other than the toplevel package.

So it seems that package-lock.json files are particularly important for two big cases:

1. application development (ensuring deploys and all systems are running the same versions everywhere)
2. ensuring local development of a package is the same between multiple maintainers or users experiencing bugs - in order to ensure everyone's seeing the same thing. This is mentioned in the yarn article as "Works On My Machine” problems.

[mapsam]

Also just confirmed with vtquery, which has the lockfile committed, that this file does not come with the tar file published to npm's registry 👍

[springmeyer]

Here are my thoughts:

• I'm appreciative of both @jfirebaugh and @mapsam sharing context and knowledge
• I'm 👍 keeping the lockfile checked in for now
• I personally need to take some time to digest the linked docs to get familiar/comfortable with lockfiles
• I'll do that in about ~ month when I'm back from vacation and will re-open this ticket if I have further questions or concerns.

[sssoleileraaa]

I agree with the convincing argument that lockfiles should always be checked in and @mapsam's comment about the 2 big reasons package-lock.json should be included.

And since npm will always ignore this file when publishing I don't see any downside to including it. Or is the problem that only npm v5.x and above will ignore it when packing and publishing? For example, if we are building node-cpp-skel using node v4.8.4 (npm v2.15.11) and it has a package-lock.json file, is it not ignored? And what is the harm in it being part of the build, besides maybe a waste of space?

[sssoleileraaa]

@springmeyer maybe it's time to reopen this issue

@GretaCB this must be the issue you were referring to when we were talking about https://github.com/mapbox/mapbox-tile-copy/pull/126#issuecomment-395098825

[millzpaugh]

@allieoop great point re npm versions and whether or not they ignore the package-lock.json.

@springmeyer sorry I must have missed this back in April - I committed the package-lock file because I thought it was a best practice for the reasons @mapsam listed. Thanks for kickstarting this conversation.

[springmeyer]

And what is the harm in it being part of the build, besides maybe a waste of space?

No harm. My hesitation was originally that I was accustomed to npm install changing based on upstream dependency changes and I was confused when I ran git diff and saw massive changes to package-lock.json. So, in short I needed to be socialized to why this is a good thing and why I should get familiar with it being the norm.

@springmeyer maybe it's time to reopen this issue

I'm feeling resolved with the support of everyone who helped explain why we want to use package-lock.json. So I don't see a need to re-open, but let me know if anything here is still needing discussion.

[mapsam]

@allieoop good point about publishing a package using NPM v2 (likely Node.js v4) that has a package-lock.json present! I just tested this out with a sample package mapsam-testing-npm. I published two versions:

• 0.1.0 published with NPM v2 and a package-lock.json file - it ships with a package-lock.json file (this is probably expected, since NPM v2 doesn't know to ignore the lockfile)
• 0.2.0 published with NPM v6 and a package-lock.json file - it does not ship with a lockfile (also expected per NPM documentation noted above)

This is an interesting case, one where a lockfile may be committed. That said, I don't see this being too harmful. If we do think this is harmful, I'd suggest it's a great reason and opportunity to completely avoid NPM v2 at this point, and move to the latest v5 or v6 and upgrade node versions to >=6 locally and in applications. We should avoid publishing with NPM v2, especially if a module has already been published with NPM >=5 and has a package-lock.json file.

[springmeyer]

re-opening. @GretaCB and I have seen some narly consequences of package-lock.json being checked in and how it relates to modules using node-pre-gyp to bundle deps. This might not been the best place to discuss this but until I find a better place, I'm re-opening here and going to be strongly cautioning the use of package-lock.json with applications that depend on node-pre-gyp (due to npm bugs like https://github.com/npm/npm/issues/17444#issuecomment-393761515)

[springmeyer]

closing again.

seen some narly consequences of package-lock.json being checked in and how it relates to modules using node-pre-gyp to bundle deps

This is still true, but hopefully I've solved this now. During https://github.com/mapbox/core-tech/issues/381 I stopped using bundledDeps which should avoid this problem. The other workaround I've used is to install with the same version of npm that was used to bundle the deps, but that is impossible to control.

Next action is to update the node-pre-gyp readme to remove the mention/requirement of bundlingDeps since as long as you drop support for node v0.10, the bundledDeps approach should not be required. The idea is that node-pre-gyp should work without bundledDeps as long as npm installs it predictably first, before trying to build the specific module and as long as npm predictably dedupes node-pre-gyp while keeping it on PATH. These conditions seem true in testing so far and node-sqlite3 has been out for several weeks without problems after unbundling.

So, back to getting used to package-lock.json without being totally blocked by it.

[springmeyer]

The idea is that node-pre-gyp should work without bundledDeps as long as npm installs it predictably first

-> https://github.com/mapbox/node-cpp-skel/pull/148

[GretaCB]

install with the same version of npm that was used to bundle the deps, but that is impossible to control

Thank you for this validation @springmeyer . I was also doing the same, and it was maddening to attempt to remember and keep this consistent throughout the dev process, especially when multitasking and working on different projects.

[springmeyer]

Thank you for this validation @springmeyer . I was also doing the same, and it was maddening to attempt to remember and keep this consistent throughout the dev process, especially when multitasking and working on different projects.

👍 @GretaCB - so hopefully unbundling will avoid this problem/gocha. The only remaining gocha is when you are working with a dependency tree to:

• upgrade on module to a version that no longer bundles dependencies
• but you "bump" the package-lock.json such that it starts bringing in a different dependency that still bundles deps (since it has not been updated).

That situation still remains until all deps that used to bundle are cleared from the node modules tree.