Add alternative command to run npm ci with cache

[mokagio]

WIP

---

• I have considered if this change warrants release notes and have added them to the appropriate section in the CHANGELOG.md if necessary.

[mokagio]

Might be worth making this configurable?

[mokagio]

The code this script was extracted from had the comment about 15 being the default. I had a quick look and couldn't find a reference. Need to look more and document.

[AliSoftware]

Found the source of that default in the npm/make-fetch-happen library, which is used by npm/cli

Suggested change

	# Default value from npm.
	# TODO: Link to source
	MAX_SOCKETS=15
	# Default value from npm. (See https://github.com/npm/make-fetch-happen/blob/main/README.md#opts-max-sockets)
	MAX_SOCKETS=15

[mokagio]

Mmmm.... 🤔

environment.bats fails in CI but passes for me locally. Usually when a test behaves like that there are differences between CI and local environment, but in this case the tests run through Docker.

[mokagio]

Note: I moved the Windows stuff to #100 because there's additional work to be done on that front and I wanted to keep the two streams separated.

Sorry for the noise that the rebase created.

[AliSoftware]

Nitpick / Question: is the goal to keep this new script being called install_npm_packages_clean? Or is this just for ease of testing the new setup using a temp separate name, but once the PR is ready for review you'll end up removing install_npm_packages and renaming install_npm_packages_clean to install_npm_packages, or alternatively (in case we want to keep the old implementation around for some time, consolidate the two scripts in one and add a --clean flag or something to allow to switch between the two behaviors?

Asking mostly because I'm not sure I like the naming of …_clean in that new command… especially since one of its role is to use npm ci + save_cache so us still using the cache doesn't feel like the term "clean" (as in "clean install") is appropriate? Or is it because, while we do use some caching, we only cache the root npm folder with the package downloads… but not the node_modules, which in a way are clean-installed? Though even if that's the case, it's not completely a clean install since if the cached downloads gets corrupted for some reason that cache will still poison the installed modules and in such case the naming of the helper might be misleadingly make us believe that it couldn't possibly be a case of cache poisoning because the command is named …_clean…

That being said, I'm not sure I have a suggestion for a better naming that would indicate "clean install of the modules, but still using cache for the downloaded packages"… yet, idk why but even if we keep the "clean" wording, it feels it'd be more logical to have it as a --clean flag than as a separate command? 🤷

[mokagio]

At the moment, I wrote this as a dedicated script to avoid breaking the npm install one.

But like you noticed, it's highly likely the only difference between the two will be whether we call install or ci (clean-install). As such, once this is proven to work (and to make a difference in terms of performance, which I'm a bit unsure about still) I was thinking about something along the lines of the --clean option (or the opposite, since ci is the recommended command for automated environment, we could opt-in using install via an option like --cache-node-modules to signify we cache node_modules, too)

Asking mostly because I'm not sure I like the naming of …_clean in that new command… especially since one of its role is to use npm ci + save_cache so us still using the cache

I wonder if this applies more broadly to our existing naming convention. install_gems, install_npm_packages, install_cocoapods... these all come with a cache but it's not clear from their name.

install_npm_packages_from_cache maybe? (install_npm_packages_loading_the_cache_if_any_first would be even clearer but it's too big of a stretch 🙃 )

[mokagio]

See also https://github.com/bloom/DayOne-Desktop/pull/66#discussion_r1980616778

[mokagio]

If/when we'll go for this consolidate implementation, we might also want to allow passing additional flags down to the npm install|ci or yarn call. See for example how Simplenote Electron has to use the --legacy-peer-deps flag. Automattic/simplenote-electron#3301

[ParaskP7]

RTO: @mokagio I am seeing this PR being included on the Apps Infra Roadmap project, but without a status, causing it show at the bottom of the Apps Infra Roadmap board within the No Status section, is that intentional? 🤔

[mokagio]

RTO: @mokagio I am seeing this PR being included on the Apps Infra Roadmap project, but without a status, causing it show at the bottom of the Apps Infra Roadmap board within the No Status section, is that intentional? 🤔

It was not intentional. I must have forgot to set a status when adding this PR to the Apps Infra Roadmap project.

[AliSoftware]

@mokagio How do you feel about undrafting this PR and pushing it to the finish line? IMHO:

• We can go replacing the install_npm_dependencies completely, instead of keeping 2 separate install_npm_dependencies vs install_npm_dependencies_clean, including because using npm ci is the official recommended approach by Nodejs when running on CI as it doesn't risk re-resolving dependencies (but instead only relies on the lockfile), so it's really the right command to use, not npm install.
• Then you can add a "breaking" entry in the CHANGELOG.md, and bump the major version, just to be on the safe side to mark the change in behavior of that command. If we ever notice that one of our client repos that uses this command doesn't work well with the new implementation that uses npm ci, we can always decide to not update it to the latest major version of the plugin while we iterate to adjust as necessary.
• We can start adopting it on repos that have already tested that npm ci works as expected (DayOne-Desktop, Simplenote-Electron, experimental-wp-dev-env, …)

If/when we'll go for this consolidate implementation, we might also want to allow passing additional flags down to the npm install|ci or yarn call. See for example how Simplenote Electron has to use the --legacy-peer-deps flag. Automattic/simplenote-electron#3301

I think the passing of unfiltered "$@" extra arguments on this line is good enough for a first iteration tbh. We can always iterate with a later PR if we want to filter allowed parameters.