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

Suggest npm ci in main readme #103

Closed
wants to merge 1 commit into from
Closed

Suggest npm ci in main readme #103

wants to merge 1 commit into from

Conversation

denis-sokolov
Copy link

Stricter and faster install command meant for CI environments. Forgive me if this has already been considered, a cursory search in the issue tracker did not find anything related.

Stricter and faster install command meant for CI environments.
@XhmikosR
Copy link

The thing is that npm ci requires a package-lock.json file to be present and not everyone might have one. Although it is the default. Just something to consider.

https://docs.npmjs.com/cli/ci.html#description

@denis-sokolov
Copy link
Author

That’s a good point that I’ve missed.

I think that it’s still a good idea to use ci. For starters, someone using GitHub Actions is reasonably likely to be more prepared. Even if they aren’t, npm displays a clear explanation what’s going wrong: “cipm can only install packages with an existing package-lock.json. [..] Run an install with npm@5 or later to generate it, then try again.” They can then generate the lockfile or replace npm ci with npm install themselves.

Going the other way around, from install to ci is less easily discoverable.

@wereHamster
Copy link

Furthermore, npm ci deletes the node_modules folder if it exists. If you have an earlier step which has restored it from cache, it'll actually make the caching pointless.

@denis-sokolov
Copy link
Author

denis-sokolov commented Mar 5, 2020

ci absolutely makes caching pointless. The PR as suggested does not forbid users to set up caching, but the examples in the documentation, the ones that do not have caching set up, will be faster.

@timoxley
Copy link

timoxley commented Jul 15, 2020

The thing is that npm ci requires a package-lock.json file to be present and not everyone might have one.

@XhmikosR

npm has created package-lock files by default for a number of years now, I think it's safe to default these to on. Anyone who is turning them off knows what they're doing.

@okuryu
Copy link

okuryu commented Jul 16, 2020

I never use package-lock.json file for libraries.
sindresorhus/ama#479 (comment)

I would like to suggest checking lock file like this.

      - name: Install
        run: |
          if [ -f package-lock.json ]; then
            npm ci
          else
            npm install
          fi

@timoxley
Copy link

I would like to suggest checking lock file like this.

Agreed

@denis-sokolov
Copy link
Author

The suggested conditional makes sense in the implementation of some automated action, but this PR talks about the documentation for the users, code samples to copy and paste. Conditional there makes the sample too big and misleads our users’ teammates as to why the lock file is sometimes there.

Seeing how there is no interest in merging this, closing.

@akd-io
Copy link

akd-io commented Jul 30, 2021

I still believe this would be useful. I personally read the docs and thought, "wait, npm ci isn't best practice anymore?" and spent half an hour finding out.

npm has created package-lock files by default for a number of years now, I think it's safe to default these to on. Anyone who is turning them off knows what they're doing.

@timoxley

I believe this is very true, add another year since this issue was closed.

@arabold
Copy link

arabold commented Jul 31, 2021

Running npm install for a CI/CD is fundamentally flawed. It might be acceptable for running tests, but if you rely on reproducible results you cannot and must not use npm install. The problem is that npm install will choose whatever version is the newest and that still matches your semver range specified in package.json. This often works well for some time, but if you suddenly get burned by a broken dependency introduced through a patch release, you potentially will spend hours trying to figure out which exact version you used before. Without a package-lock.json you have zero traceability and virtually no chance of reproducing the exact previous build ever again. Quite frankly, anybody suggesting otherwise never had to deal with a broken production build because a fricking dev dependency broke on the CI/CD side 🙄

Anyhow, I'll stop ranting now. I'm with the original author of this post that the documentation should be updated to suggest npm ci instead of npm install 💯. Even if that's just cosmetics for some people, many new users will simply copy-paste from whatever is stated in the README.md.

Side note: I usually use npm ci --prefer-offline --no-audit on my CI/CD builds as it is slightly faster - no online check if the packages are already cached locally and no auditing, as there probably won't be anybody reading the audit log on the CI/CD machine anyway.

tl;dr. Please update the documentation to state npm ci instead of npm install. 🙏

@akd-io
Copy link

akd-io commented Aug 1, 2021

Great insight @arabold, thanks a lot! ❤️

@jonkoops
Copy link
Contributor

jonkoops commented Sep 3, 2021

Furthermore, npm ci deletes the node_modules folder if it exists. If you have an earlier step which has restored it from cache, it'll actually make the caching pointless.

This is actually false. npm ci does indeed remove the node_modules directory, but that does not make the caching pointless. In fact, the node_modules are not cached at all but rather the global NPM cache directory (as can be seen here).

@jonkoops
Copy link
Contributor

jonkoops commented Sep 3, 2021

I've opened up a PR under #326 to improve the documentation with these sensible defaults (also for Yarn and PNPM).

deining pushed a commit to deining/setup-node that referenced this pull request Nov 9, 2023
…ions#103)

Bumps [eslint-plugin-prettier](https://github.com/prettier/eslint-plugin-prettier) from 3.1.4 to 3.2.0.
- [Release notes](https://github.com/prettier/eslint-plugin-prettier/releases)
- [Changelog](https://github.com/prettier/eslint-plugin-prettier/blob/master/CHANGELOG.md)
- [Commits](prettier/eslint-plugin-prettier@v3.1.4...v3.2.0)

Signed-off-by: dependabot[bot] <support@github.com>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants