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

Do not encourage to publish package-lock in the source code version control as a general practice #810

Open
cseas opened this issue Oct 27, 2023 · 9 comments
Labels
content Issues or PRs related to the content of the docs

Comments

@cseas
Copy link

cseas commented Oct 27, 2023

Opening this here again as the repository of the earlier reported issue got archived.
npm/npm#20603

The npm documentation mentions it clearly that package-lock is always meant to be committed, leaving library authors unaware of the risk that their consumers might install different dependency versions than what they're using.

The documentation should be changed to mention that since package-lock files are ignored in published npm packages, library authors should exercise caution when choosing to commit them to source code.

Examples:

  1. https://docs.npmjs.com/cli/v6/configuring-npm/package-locks#using-locked-packages
It is highly recommended you commit the generated package lock to source control
  1. https://docs.npmjs.com/cli/v10/configuring-npm/package-lock-json#description
This file is intended to be committed into source repositories

Article with reasoning:
https://gajus.medium.com/stop-using-package-lock-json-or-yarn-lock-909035e94328

cc: @gajus

@MylesBorins
Copy link
Contributor

MylesBorins commented Oct 27, 2023

I disagree with this suggestion. While it is true that when a package is a dependent you are going to have a different tree, there is a large benefit to consistent development environments.

Yes, there is inconsistency when people install the package, but just because there are scenarios where there is a lack of consistency doesn't mean there is no value in a development environment that is mostly reproducible. Having consistency when collaborating across teams, reviewing PRs, and debugging 3rd party bug reports removes a ton of randomness and edge cases.

It is definitely possible to have a test run in CI that includes --package-lock=false if you want to test what happens when you install w/o the lock, but imho that is far more valuable as a daily task rather than the default scenario. Every tree is going to end up with a different layout / resolution of the tree based on the various other dependencies (prod / dev / peer), so trying to test out for all potential iterations is impossible... what is possible is ensuring you have consistency during development.

I'll leave this open for more discussion, but I would not approve any changes to the current documentation that would remove this recommendation.

@ljharb
Copy link
Contributor

ljharb commented Oct 27, 2023

I make an .npmrc in all my projects with package-lock=false and i gitignore lockfiles for this reason.

Defaulting a lockfile was the wrong decision in npm 5 and it’s the wrong decision now. “A consistent development environment” is a bug - in the few scenarios where you need reproducibility, you can use --before to obtain it just as easily as you could use package-lock to disable the lockfile, but the better default is the experience consumers get, even if it’s inconsistent.

@lukekarrys lukekarrys added the content Issues or PRs related to the content of the docs label Oct 31, 2023
@DanKaplanSES
Copy link

DanKaplanSES commented Jan 18, 2024

The documentation should be changed to mention that since package-lock files are ignored in published npm packages, library authors should exercise caution when choosing to commit them to source code.

Wow, I never considered that. In this context, would a shrinkwrap file be recommended? That page says:

It's strongly discouraged for library authors to publish this [shrinkwrap] file, since that would prevent end users from having control over transitive dependency updates.

While that makes sense, it ignores the issue you've brought up so, as far as I can tell, it's only half the story.

@ljharb
Copy link
Contributor

ljharb commented Jan 18, 2024

Publishing a shrinkwrap file is exceedingly user-hostile, because it prevents end users from getting transitive dependency updates.

In an app - something not published - shrinkwrap and package-lock are identical, and should always exist.

@DanKaplanSES
Copy link

DanKaplanSES commented Jan 18, 2024

Publishing a shrinkwrap file is exceedingly user-hostile, because it prevents end users from getting transitive dependency updates.

But it only prevents end-users from getting that library's transitive dependencies, right? That's my interpretation and if it's incorrect I think the documentation should clarify (but that's unrelated to this issue). If I am correct, how can that affect the end user, unless the library failed in some other way (e.g., should have used peer dependencies but didn't).

@ljharb
Copy link
Contributor

ljharb commented Jan 18, 2024

Yes, that's right - it affects the end user because the dependencies of your library can't be deduped, or upgraded, which means if there's a bug or security vulnerability, they're stuck with what's in the shrinkwrap file.

@DanKaplanSES
Copy link

Yes, that's right - it affects the end user because the dependencies of your library can't be deduped, or upgraded, which means if there's a bug or security vulnerability, they're stuck with what's in the shrinkwrap file.

Ah, gotcha. That makes sense.

@matthias-ccri
Copy link

matthias-ccri commented Aug 27, 2024

“A consistent development environment” is a bug

I'm sorry but I have to chuckle a bit here. You gotta admit, it is a little funny to say that stability and order is a bug. Sure, you want to catch incompatibilities sooner. But entertain the opposing position for a second --- some developers want to develop without surprises. Maybe stability is a good thing?

Let's also explore what is the easier option:

where you need reproducibility, you can use --before to obtain it just as easily ...

Maybe using --before is easy, but it's not just as easy a having a lockfile. You can't commit it or share it as easily. You have to get your CI to run it somehow. (correction: it can be committed by putting it in an npmrc file) I assume we're talking about debugging certain dependency combinations. This is where a lockfile shines, because a lockfile is an easy way to snapshot a dependency tree in time. Create the dependency tree with --before, then capture it in a lockfile, and push it to a branch. Golden.

It's actually easy on the other end, if you have a lockfile and want to install bleeding edge deps. Just delete the lockfile and install from scratch. That's just as easy.

Moreover, sorry if this is preachy, but if you want to catch errors, you should do it in an organized way:

  1. decide that you want to catch errors
  2. install bleeding edge deps and look for errors

The alternative is this:

  1. try to do work
  2. be surprised by some random breakage
  3. be forced to deal with it

Which one is more organized? If npm removes the lockfile by default, it would make users randomly deal with bleeding edge bugs. Is it worth it?

You also said something that I need to pick at:

the better default is the experience consumers get

The question is: which consumers?

  • Consumers that just created a new project and are getting the bleeding edge versions of all deps?
  • Consumers that have existing projects and are adding your package?
  • Consumers that have already installed your package at some previous point in time, and are installing or updating other dependencies?

Consumers have lockfiles too, so your consumers can have any variation or combination of deps. Also, consumers don't install your devDependencies, so if you want your library project's node_modules to mirror a consuming project, it's going to be hard. You might as well set up a downstream project just to test the package.

The point is: given the variability in consumer installation scenarios, a lockfile is the best way to capture each scenario in a branch, so it can be developed and tested in an orderly way.

@ljharb
Copy link
Contributor

ljharb commented Aug 27, 2024

@matthias-ccri you can put before=2024-08-27 in .npmrc and commit it, and it'll work in CI too. so yeah, it's just as easy.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
content Issues or PRs related to the content of the docs
Projects
None yet
Development

No branches or pull requests

7 participants
@ljharb @DanKaplanSES @MylesBorins @lukekarrys @matthias-ccri @cseas and others