package-manager-best-practices icon indicating copy to clipboard operation
package-manager-best-practices copied to clipboard

npm: recommendation for standalone CLI projects

Open yoavain opened this issue 3 years ago • 12 comments

The recommendation for a standalone CLI project should be that when publishing npm-shrinkwrap.json file, it should only contain dependencies and not dev-dependencies. Dev dependencies are best practice for the developer(s) of the CLI, for stability and consistency between environments. However, running npm shrinkwrap will only take the package-lock.json file and rename it, resulting in a largenpm-shrinkwrap.json which contains both dependencies and dev-dependencies. When a consumer installs this CLI it will install all dependencies, and not only those needed for runtime.

My recommendation:

  1. Committing the package-lock.json file to the CLI project (never the npm-shrinkwrap.json)
  2. In the publish pipeline do the following

prepack script should:

  • Remove dev dependencies (i.e. npm prune --production)
  • Delete package-lock.json file
  • Running npm shrinkwrap

pack will then take the "slim" npm shrinkwrap

postpack script should revert changes:

  • checkout package-lock.json
  • delete the npm-shrinkwrap.json

The reason to delete the package-lock.json file is that npm prune --production only deletes files from the node_modules folder and does not update the package-lock.json file, and the npm shrinkwrap only rename the package-lock.json to npm-shrinkwrap.json if it exists. When package-lock.json does not exist, the npm-shrinkwrap.json file is created from the existing dependencies tree (which after the prune command represents the committed package-lock.json file, without the dev dependencies)

There's also the save the planet reason. An example for the impact of such a change to a single package in terms of storage: https://github.com/barzik/branch-name-lint/pull/50

yoavain avatar Sep 03 '22 09:09 yoavain

That is an impractical recommendation, since it requires pruning, and it’s irrelevant since dev deps won’t be installed anyways.

It’s not the community’s job to keep things small, it’s npm’s.

ljharb avatar Sep 03 '22 15:09 ljharb

That is an impractical recommendation, since it requires pruning, and it’s irrelevant since dev deps won’t be installed anyways.

It’s not the community’s job to keep things small, it’s npm’s.

Yeah, I figured it's an unpopular opinion.

I think that since publish is usually the last thing you do in a CI/CD pipeline, it's fine to prune dev dependencies. Just like you would do before you pack a deployable without dev dependencies, for example, a server. As for the second part, npm install installs everything in the shrinkwrap file, not only "runtime" dependencies. You can check for yourself:

npm init -y
npm install -D branch-name-lint

it installs more than 500 dependencies that include all dev dependencies that this package uses. (instead of only 70 runtime dependencies).

We can argue if this is an npm issue or not, but in practice, having dev dependencies in the shrinkwrap file results in redundant dependencies in your node_modules folder which means it also adds time to your CI/CD pipeline during npm install. So I think it can count as best practice...

yoavain avatar Sep 03 '22 16:09 yoavain

Ah, right - run npm shrinkwrap --only=prod iirc?

ljharb avatar Sep 03 '22 22:09 ljharb

I remember looking for such a flag when I wanted this behavior, and couldn't find such. Nothing in the documentation either.

AFAIK, if there's a package-lock.json file, npm shrinkwrap renames it to npm-shrinkwrap.json. If there isn't a lock file, npm-shrinkwrap.json is created from the existing node_modules tree.

yoavain avatar Sep 04 '22 03:09 yoavain

Try it. There's also npm shrinkwrap --omit=dev.

ljharb avatar Sep 04 '22 04:09 ljharb

Already have. Same result. The log to the console:

npm notice package-lock.json has been renamed to npm-shrinkwrap.json

yoavain avatar Sep 04 '22 04:09 yoavain

npm shrinkwrap --production? Airbnb has used one of these for years, so although I can't remember the exact incantation, I'm quite confident it's a feature of shrinkwrap :-)

ljharb avatar Sep 04 '22 04:09 ljharb

Looks like this is a confirmed bug with the npm CLI and dev dependencies should not be installed when a project has a npm-shrinkwrap.json file https://github.com/npm/cli/issues/4323

erezrokah avatar Sep 04 '22 07:09 erezrokah

Looks like this is a confirmed bug with the npm CLI and dev dependencies should not be installed when a project has a npm-shrinkwrap.json file npm/cli#4323

Interesting. Not sure I agree with their interpretation of the bug. I think the bug is in the npm shrinkwrap command and not in the npm install. I don't see any reason the npm-shrinkwrap.json should contain dev dependencies at all. That's what package-lock.json is for. But again, I might be in the minority here.

So maybe we can settle on "best workaround practice"

yoavain avatar Sep 04 '22 11:09 yoavain

Is there any consensus emerging from the discussion?

Will the fix in https://github.com/npm/cli/issues/4323 address the concern without the need to update the recommendation?

laurentsimon avatar Sep 09 '22 21:09 laurentsimon

Is there any consensus emerging from the discussion?

Will the fix in https://github.com/npm/cli/issues/4323 address the concern without the need to update the recommendation?

I think that a fix to the issue (as they interpret the bug) will only be a partial solution. As long as the shrinkwrap file contains all dev dependencies, it will still depend on npm update adoption by users. I believe that most users will not get the fix, and will still have all dev dependencies installed.

yoavain avatar Sep 13 '22 07:09 yoavain

I think the npm best practices document needs to identify best (workaround) practices, given the mechanisms that currently exist and the limited patience of developers :-).

If there's no practical/easy way to do something in a package manager (like npm), or its defaults are "wrong", then I recommend creating a separate little project within this OpenSSF WG to resolve it. Once it's resolved in the tool(s), we can update the guide to match :-).

That said: It'd be really helpful if we could clearly state what that problem is, so that a separate little project can work on it. Anyone want to take a crack at it? First attempt (please fix):

It should be easy in npm to create a standalone CLI project that specifically locks its runtime dependencies but not its development dependencies (dev-dependencies). Currently you can run npm shrinkwrap but this results in a large npm-shrinkwrap.json which contains both dependencies and dev-dependencies. npm currently intends to work around this with https://github.com/npm/cli/issues/4323 where the dev-dependencies aren't installed, but this is confusing and misleading - including such unused information is likely to confuse users and tools.

david-a-wheeler avatar Sep 13 '22 11:09 david-a-wheeler