• Why npm install instead of npm ci? (my biggest issue here)

This primarily addresses an issue with local development, where one of us forgets to run npm install after pulling changes that updated package.json / package-lock.json: npm run build first compiles the TypeScript files to JavaScript and then bundles up the JavaScript files using esbuild. The dependencies get included in the bundles generated by esbuild. The resulting bundles are checked into the repo (in the lib directory). If we are working on something and open a PR with changes resulting from npm run build with outdated dependencies, then the CI will fail on the PR.

In CI, we already run npm ci.

• Is it expected by the users that build now installs dependencies? It probably is, there's certainly precedence for doing it, but the change itself is semantically huge. Given the number of active contributors, we are good though I think.

CI will always fail on a PR if you bundled the JS files with outdated dependencies. So practically speaking, you never want to not run npm install if the dependencies have changed and you are updating the bundles.

In other words: if you run npm run build and didn't run npm install when dependencies have changed, then you are reverting the bundles in lib back to older versions.

I think we need to print something in the shell script. Otherwise, a stale node_module is going to cause some npm run build mystery for a developer some day.

check-node-modules.sh: node_modules appears to be up to date

check-node-modules.sh: node_modules appears to be outdated, doing npm install

Perhaps -- I had considered it and there's certainly no harm in adding extra messages, but the output from npm install is already pretty clear about what's happening.

Thanks for elaborating.

I'm still a bit iffy on the fact that a forgotten npm ci in our workflow files now is hidden by the npm install that will happen instead (I'm assuming our workflows use npm run build).

but the output from npm install is already pretty clear about what's happening.

Its the other way around I worry about. The timestamp check is just one (very good) indicator for outdated dependencies, combined with the silence when that indicator falsely holds, we can end up in a mystery situation.

Consider this scenario:

• developer effectively deletes a module folder in node_modules due to git stuff
• developer is used to npm run build doing the right thing - and specifically does not know about the timestamp check
• npm run build now silently does not install the deleted dependency

I think the check-node-modules.sh: node_modules appears to be up to date would alleviate that a bit.

Consider this scenario: [..]

I am not convinced that this is really a problem, because actually building the bundle would then fail because of the missing dependency. For example, running rm -rf node_modules/@actions and then node build.mjs fails with:

[...]
Error: Build failed with 53 errors:
src/actions-util.ts:4:22: ERROR: Could not resolve "@actions/core"
src/actions-util.ts:5:28: ERROR: Could not resolve "@actions/exec/lib/toolrunner"
src/actions-util.ts:6:24: ERROR: Could not resolve "@actions/github"
src/actions-util.ts:7:20: ERROR: Could not resolve "@actions/io"
src/analyze-action-post.ts:8:22: ERROR: Could not resolve "@actions/core"
[...]

I would assume that this would prompt any of us to run npm install, no matter how used we might be to npm run build normally doing it for us. The only scenario I can imagine where this would be confusing is if you're not familiar with npm / JavaScript development, but then you'd probably be having a hard time generally.

The thinking behind the addition in this PR is not to simplify the development workflow, but to protect us from forgetting to run npm install by accident. The scenario above would have been essentially the same before the changes in this PR, except that we now avoid most of these cases by running npm install automatically when we detect that the dependencies might be missing or outdated.

Nonetheless, I have opened #3155 which adds log messages + skips the script when it detects that it is running in an Actions workflow.