Skip to content
This repository has been archived by the owner on Oct 9, 2023. It is now read-only.

npm: recommendation for standalone CLI projects #35

Open
yoavain opened this issue Sep 3, 2022 · 12 comments
Open

npm: recommendation for standalone CLI projects #35

yoavain opened this issue Sep 3, 2022 · 12 comments

Comments

@yoavain
Copy link

yoavain commented Sep 3, 2022

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:
barzik/branch-name-lint#50

@ljharb
Copy link
Member

ljharb commented Sep 3, 2022

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.

@yoavain
Copy link
Author

yoavain commented Sep 3, 2022

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...

@ljharb
Copy link
Member

ljharb commented Sep 3, 2022

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

@yoavain
Copy link
Author

yoavain commented Sep 4, 2022

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.

@ljharb
Copy link
Member

ljharb commented Sep 4, 2022

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

@yoavain
Copy link
Author

yoavain commented Sep 4, 2022

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

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

@ljharb
Copy link
Member

ljharb commented Sep 4, 2022

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 :-)

@erezrokah
Copy link
Contributor

erezrokah commented Sep 4, 2022

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

@yoavain
Copy link
Author

yoavain commented Sep 4, 2022

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"

@laurentsimon
Copy link
Contributor

Is there any consensus emerging from the discussion?

Will the fix in npm/cli#4323 address the concern without the need to update the recommendation?

@yoavain
Copy link
Author

yoavain commented Sep 13, 2022

Is there any consensus emerging from the discussion?

Will the fix in npm/cli#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.

@david-a-wheeler
Copy link

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 npm/cli#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.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants