Add alternative command to run `npm ci` with cache #96

mokagio · 2024-05-30T07:16:32Z

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 · 2024-05-30T07:17:38Z

bin/install_npm_packages

@@ -2,13 +2,13 @@

 # Ensure package.json is present
 if [ ! -f package.json ]; then
-    echo "No valid package.json file found"
+    echo "No package.json found!"


Me being pedantic:

No validation made on package.json

We know that package.json is a file

mokagio · 2024-05-30T07:18:11Z

bin/install_npm_packages_clean

+fi
+
+# Ensure package-lock.json is present
+# TODO: What to do with yarn-based projects? Make the hash on lockfile optional?


I guess a project using yarn would not call a command with npm in the name?

mokagio · 2024-05-30T07:18:59Z

bin/install_npm_packages_clean

+# TODO: This comes from Studio. Figure out if it's standard in Node or not.
+if [ -d patches ]; then
+  PATCHES_HASH=$(hash_directory patches/)
+else
+  PATCHES_HASH=nopatch
+fi


Even if it isn't a standard approach, it might be worth leaving it here as it adds basically no overhead

FYI the implementation was inspired by this recommendation: https://www.npmjs.com/package/patch-package#circleci ; so I guess we could consider that "standard" in that regard?

Oh, nice.

I'll add a link to it and remove the todo. Thanks!

mokagio · 2024-05-30T07:19:25Z

bin/install_npm_packages_clean

+LOCAL_NPM_CACHE=./vendor/npm
+mkdir -p $LOCAL_NPM_CACHE
+echo "--- :npm: Set npm to use $LOCAL_NPM_CACHE for cache"
+npm set cache $LOCAL_NPM_CACHE
+echo "npm cache set to $(npm get cache)"


Might be worth making this configurable?

mokagio · 2024-05-30T07:20:21Z

bin/install_npm_packages_clean

+  # Default value from npm.
+  # TODO: Link to source
+  MAX_SOCKETS=15


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.

See #96 (comment) Co-authored-by: Olivier Halligon <[email protected]>

mokagio · 2024-06-03T01:25:49Z

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.

If a project uses `yarn` it should not use the `npm ci` command in CI!

See #96 (comment) Co-authored-by: Olivier Halligon <[email protected]>

mokagio · 2024-06-03T03:48:00Z

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 · 2024-06-06T12:11:24Z

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

See #96 (comment) Co-authored-by: Olivier Halligon <[email protected]>

mokagio linked an issue May 30, 2024 that may be closed by this pull request

Improve install_npm_packages based on Studio build process #94

Open

1 task

mokagio mentioned this pull request May 30, 2024

Run lint and tests in Buildkite CI Automattic/simplenote-electron#3206

Closed

mokagio commented May 30, 2024

View reviewed changes

mokagio added a commit that referenced this pull request Jun 3, 2024

Add documentation for patches support to npm ci command

bc01425

See #96 (comment) Co-authored-by: Olivier Halligon <[email protected]>

mokagio and others added 5 commits June 3, 2024 13:34

Remove a trailing whitespace

f3e98ac

Tweak error messages in install_npm_packages

53d9684

Add command to run npm ci

84d6b95

Remove note about yarn from npm ci command

574c11b

If a project uses `yarn` it should not use the `npm ci` command in CI!

Add documentation for patches support to npm ci command

154bde2

See #96 (comment) Co-authored-by: Olivier Halligon <[email protected]>

mokagio force-pushed the mokagio/npm_ci branch from c4cda93 to 154bde2 Compare June 3, 2024 03:35

AliSoftware reviewed Jun 4, 2024

View reviewed changes

mokagio added a commit that referenced this pull request Aug 28, 2024

Add documentation for patches support to npm ci command

0baaf8d

See #96 (comment) Co-authored-by: Olivier Halligon <[email protected]>

mokagio mentioned this pull request Feb 3, 2025

Add PowerShell command to setup Windows environment #100

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add alternative command to run `npm ci` with cache #96

Add alternative command to run `npm ci` with cache #96

mokagio commented May 30, 2024

mokagio May 30, 2024

mokagio May 30, 2024

mokagio May 30, 2024

AliSoftware May 30, 2024

mokagio Jun 3, 2024

mokagio May 30, 2024

mokagio May 30, 2024

mokagio commented Jun 3, 2024

mokagio commented Jun 3, 2024

AliSoftware left a comment

mokagio commented Jun 6, 2024

Add alternative command to run npm ci with cache #96

Are you sure you want to change the base?

Add alternative command to run npm ci with cache #96

Conversation

mokagio commented May 30, 2024

mokagio May 30, 2024

Choose a reason for hiding this comment

mokagio May 30, 2024

Choose a reason for hiding this comment

mokagio May 30, 2024

Choose a reason for hiding this comment

AliSoftware May 30, 2024

Choose a reason for hiding this comment

mokagio Jun 3, 2024

Choose a reason for hiding this comment

mokagio May 30, 2024

Choose a reason for hiding this comment

mokagio May 30, 2024

Choose a reason for hiding this comment

mokagio commented Jun 3, 2024

mokagio commented Jun 3, 2024

AliSoftware left a comment

Choose a reason for hiding this comment

mokagio commented Jun 6, 2024

Add alternative command to run `npm ci` with cache #96

Add alternative command to run `npm ci` with cache #96