Update recommendations for lockfiles

[laurentsimon]

This PR takes into account the discussion in #24 and #10.

It may need a few adjustments once #10 (comment)
is answered.

@ljharb , @lirantal, @soldair, @erezrokah, @wesleytodd would love your input

[ljharb]

Thanks, looks great!

[wesleytodd]

This is not the best practice, similar to the shrinkwrap discussion, it depends on the type of project. So only applications should define a package-lock.json and commit it to their repos.

If libraries are using lockfiles they can accidentally publish versions which were only tested with outdated dependencies, so end users might break when they do a fresh install. The goal for library authors should be to have a CI system which does a "fresh" install so that a new user that just runs npm i your-lib will atleast at the time of publishing get the same experience you did in your tests.

For comparison, an application should use a lock file because for your users to "get the same experience as when authored" you need even rebuilds of the same source to produce consistent results. So the lockfile means a build from the same commit will get the same experience you intended at the time of publishing. And you as the author typically control the full experience, as there is no "client building" of the app code like there is with a lib.

[laurentsimon]

you're right I botched the part on libraries. I was trying to say the following:

1. A package-lock.json should be used and committed to the repo for all projects unless they use an npm-shrinkwrap (reasoning: benefits developers' machines as per here). Contributors and maintainers should be encouraged to run npm ci and npm install-ci-test locally.
2. Libraries (or standalone CLIs that end up in other projects) should always ignore the lock file when running tests in CI (e.g. via npm install --no-package-lock) and should be careful to run these tests in a non-privileged environment.
3. Applications projects should run CI via npm ci and npm install-ci-test, and should also pay attention to using the less-privileged principles for their CI runs

I'm trying to reconcile the comments in #24 (lockfiles to mitigate malicious dependencies on dev machines) and the fact that it should not be used on libraries (or in projects that end up in other projects' manifest / lockfiles).

An alternative to 1 above would be to tell contributors to define a lockfile locally (in combination with .gitignore). However updates to this file would be manual and difficult to keep track of. Having it in the repository benefits from the commit history of the repo.

Looking forward to your feedback / suggestion. Thanks again!

[laurentsimon]

friendly ping for feedback
@wesleytodd

[lirantal]

Not sure where this captures the actual legitimate use of shrinkwrap files, which is when building and shipping standalone CLIs.

[laurentsimon]

the text above was just a diff from the PR's content. shrinkwrap was already discussed. Can you take a look at the section If a project is a standalone CLI:?

[lirantal]

Aaa yes I see it there now. Sorry, jumped too fast on this :-)

[laurentsimon]

@wesleytodd do you have further feedback? Are you OK with the latest changes?

[wesleytodd]

Really sorry for dropping off here, I totally missed the notification.

I think what landed in the "recommendations" section is technically correct, so glad to see it got sorted out. One thing I do feel here is that it is a quite complicated set of recommendations. I sent the link to a team member at work and they basically just said "I will just remove the package-lock". I think this will be a common response because the guide is technically correct at the cost of being very complicated.

Anyway, like you said below:

I think developers will do in practice regardless of what we say (e.g. when troubleshooting/repdoducing a problem). I think the intention is to raise awareness and encourage users to run these non-locked tests in CI/less-privileged env when possible.

👍

[laurentsimon]

I don't disagree that we could make the recommendation simpler. Do you think we should include more examples, and possibly links to an example repository which contains lock files, a workflows example, etc?

[laurentsimon]

I've made additional updates to the doc. PTAL

[jeffmendoza]

Looking good. Should all the instances of "except when intentionally adding /removing a dependency" add "updating" as well?

[laurentsimon]

Looking good. Should all the instances of "except when intentionally adding /removing a dependency" add "updating" as well?

updating should be done via a tool like dependabot, so I left it out. Maybe we could add a line to handle update and refer to the section that ask to use a tool?

[jeffmendoza]

Not everyone has flawless tests that they can rely on. A common workflow might be:

1. Notice a bot PR to update a dep
2. Do an npm update locally and test out some functionality
3. Approve and merge the bot PR

[laurentsimon]

Not everyone has flawless tests that they can rely on. A common workflow might be:

1. Notice a bot PR to update a dep
2. Do an npm update locally and test out some functionality
3. Approve and merge the bot PR

SGTM. Please update the doc

[ljharb]

to be fair tho, this is also true for developers, despite the (relatively small) risk to their local machines.

[laurentsimon]

I don't entirely disagree. I think developers will do in practice regardless of what we say (e.g. when troubleshooting/repdoducing a problem). I think the intention is to raise awareness and encourage users to run these non-locked tests in CI/less-privileged env when possible.

Let me know if you think we should change it or if this is acceptable as-is.

[jeffmendoza]

I'm going to merge this as it hasn't received comments in a while, and we can re-open any issue if there are still concerns.

[erezrokah]

Thanks everyone for working on this and sorry for the lack of response and feedback, I was busy attending to a new family member 👶

Everything looks great, I followed up with minor fixes in #26