[WIP] tools: s/npm install/npm ci

[MylesBorins]

npm ci is substantially faster than npm install.

Can't see why we wouldn't want to do this... only catch would be if we don't support package-lock... but we have those currently checked in afaik

[nodejs-github-bot]

@MylesBorins build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/162/pipeline

[MylesBorins]

Before w/ hot cache

added 228 packages from 101 contributors and audited 346 packages in 6.726s
found 0 vulnerabilities

Markdown linter: installing remark-preset-lint-node into tools/
npm WARN You are using a pre-release version of node and things may not work as expected

added 54 packages from 13 contributors and audited 300 packages in 5.312s

after w/ cold cache

make lint-md-build
Markdown linter: installing remark-cli into tools/
added 160 packages in 7.724s
Markdown linter: installing remark-preset-lint-node into tools/
added 53 packages in 3.015s

After w/ hot cache

Markdown linter: installing remark-cli into tools/
added 160 packages in 1.305s
Markdown linter: installing remark-preset-lint-node into tools/
added 53 packages in 0.862s

[targos]

I think you can remove --no-package-lock now

[lpinca]

LGTM with @targos's comment addressed.

[MylesBorins]

ci: https://ci.nodejs.org/job/node-test-pull-request/15638/

[MylesBorins]

It would appear that this PR may actually slow things down based on how things are currently set up. I noticed when testing on master that we are currently calling to make test-doc-build (or whatever the job is called) everytime we run make test. This ends up essentially being a no-op with npm install, but it will introduce some latency. Going to dig in to see if there is a better way to do this.

[Trott]

Don't forget to update vcbuild.bat too. (You can dismiss this review once that's happened.)

[Trott]

A second blocker on this is that make tools/doc/node_modules/js-yaml/package.json will pollute checked-in files currently. (We float patches on the marked module--something we probably started doing only after this PR was opened already--anyway, that make`` command will overwrite those patches if we use npm ci. This will be a non-issue if/when we move to remarkinstead ofmarked`. There's a PR for that.)

So if someone using a source tarball runs make doc-only, it will overwrite marked with an unpatched version and produce a broken all.html file. Subsequent runs will have breakage in other doc files too. 😱

Because of this, I'm going to label this one blocked.

[MylesBorins]

@Trott should we perhaps close this?

[Trott]

@MylesBorins My second objection has been fixed (I think by work done by @rubys). So it's just about vcbuild.bat at this point.

This still might be worth closing (or at least modifying) because there is now a run-npm-ci task that can allow us to phase in npm ci where it makes sense but avoid it where it doesn't.

[refack]

@MylesBorins I've followed up on this in #22399

[refack]

Completed in d320511