Add script to check whether npm i needs to be run by mbg · Pull Request #3154 · github/codeql-action (original) (raw)
And add it to the build command
Risk assessment
For internal use only. Please select the risk level of this change:
- Low risk: Changes are fully under feature flags, or have been fully tested and validated in pre-production environments and are highly observable, or are documentation or test only.
Merge / deployment checklist
- Confirm this change is backwards compatible with existing workflows.
- Consider adding a changelog entry for this change.
- Confirm the readme and docs have been updated if necessary.
Copilot AI review requested due to automatic review settings
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR adds a script to automatically check if npm install needs to be run before building the project. The change aims to ensure dependencies are up-to-date by detecting when node_modules is missing or when package-lock.json is newer than the installed dependencies.
- Adds a bash script that checks if npm install is needed and runs it automatically
- Integrates the check into the build command to ensure dependencies are current before building
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| scripts/check-node-modules.sh | New script that checks if node_modules exists and if package-lock.json is newer than installed dependencies |
| package.json | Updates the build command to run the dependency check script before transpiling |
and add it to the build command
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems reasonable
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Surprisingly simple optimization. But it is a semantically large one.
- Why
npm installinstead ofnpm ci? (my biggest issue here) - Is it expected by the users that
buildnow 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.
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
- Why
npm installinstead ofnpm 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
buildnow 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 buildmystery 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 builddoing the right thing - and specifically does not know about the timestamp check npm run buildnow 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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})